Hi,

I'm currently involved in some TRegistry bugs and regressions. Personally I don't use TRegistry in any of my programs. Also I mostly use Lazarus, so most most of the issues don't affect me.

However I would like to share som observations and thoughts.

TRegistry on Windows now (3.2+) uses Unicode API.
String input parameters in the various methods get "promoted" to Unicode and then the API is called.
Returned string values however are mostly encode in UTF8, by explicitely calling Utf8Encode(SomeUnicodeString). Is that (enforce UTF8 encoding) by design?
(The Ansi to Unicode was done via UTF8Decode which is definitively wrong and is fixed by now.)

On Lazarus, this no problem, since by default all strings are UTF8 encoded, so all conversions are lossless.

In a plain fpc program though on Windows, default encoding is the current codepage (cp1252 in my case) and information will get lost when you process the result further.

On non-Windows platforms all data in the registry is (supposed to be) UTF8-encoded in a XML file.

Since the registry interfaces with a Unicode API (Windows) or UTF8 API (all other platforms), would it maybe make sense to use UnicodeString parameters throughout TRegistry? (UnicodeString because this is primarily used on Windows, otherwise I'ld suggest UTF8String.)

Now all conversions from and to UnicodeString are hidden from the programmer, so he/she cannot know that dataloss due to conversion may occur.
Using UnicodeString parameters will make the caller aware (if he/she uses AnsiStrings as parameters) that conversion will happen.

Pro's
- simpler and more consistent code in the Windows implementation of TRegistry
- awareness of conversion for the programmer

Con's
- people will complain about the warnings
- XMLReg implentation needs Utf8Encode/Utf8Decode (currently there is no conversion there: even if system codepage is not UTF8, the XML file claims it is, so this might be wrong as is)
- UnicodeStrings are slower (my guess is that acessing the API itself is slower than the Pascal code in the registry methods)
- We do not have a Unicode TStringList (for
ReadStringList/WriteStringList methods)

Whilst I know that hardly any fpc devel uses TRegistry, without getting your thoughts and opinions on this matter it makes no sense to suggest patches implementing such a big change.

I think Lazarus users are the main TRegistry users, so I would keep current behaviour for the public API. Where possible add overloads that use a unicodestring, and let the UTF8 one call the unicode one.

Under the hood, you can then completely switch to UnicodeString if that makes it easier.

The XML registry also works with Unicode string, since DOM uses unicodestring.

Michael.

Op 2/25/2019 om 9:27 PM schreef Michael Van Canneyt:

The current situation does not improve anything for Lazarus users that set the default encoding to utf8 (aka utf8hack)

If I look into e.g. registry.pp, the only use of utf8encode there is  like this:

var  s : string;
        u:unicodestring;
s:=utf8encode(u);

which, IF lazarus is used in the default utf8 mode is equivalent to

s:=u;

 So currently this utf8encode only frustrates the situation for people that don't set the default codepage to utf8?

If I'm wrong, what is the exact behaviour that you want to keep?

If I understood the OP correct, he wants to change the use of "string" arguments in the public API to unicodestring.

That changes a lot.

Contrary to popular belief, the conversion will not automatically be correct, and will produce errors.

(See e.g. https://bugs.freepascal.org/view.php?id=35113 for a similar situation where part of the error is that the lazarus user must explicitly call Utf8Decode.)

So my proposal is to leave the public API as-is, using string, adding unicode string overloads where possible/useful.

Internally, convert to whatever fits best.

if the internal routines are easier to maintain/understand if they use unicode string throughout: refactor them to use unicode.

Michael.

If you just recompile unmodified non-unicode app for Windows, string data loss is imminent if the data is not encoded using the current ANSI encoding. Even if you use UnicodeString parameters and result for registry functions.

Just adding a single line to the beginning of the app will solve the data loss:
DefaultSystemCodePage:=CP_UTF8;

I would leave the registry parameters and results as "string" as it is done in other parts of RTL/FCL. For performance critical functions it is possible to add an overloaded versions of functions which accepts UnicodeString parameters. IMO the registry access is not the time critical.

Yuriy.

My proposal exactly.

But inner workings can be made to use Unicode, because the underlying APIs are using unicode: The *W registry calls on windows, XML DOM on other systems.

Michael.

Op 2/26/2019 om 11:04 AM schreef Michael Van Canneyt:

(seems to indicate that that is utf8string, and not "string" with lazarus defaultsystemcodepage to utf8. I can imagine anything

wrong inserting a conversion over "string" and thus mangling the result)

Leave as is:  only works with lazarus hack. Other people wo

upgrade to unicodestring :works for both cases (regardless of defaultsystemcodepage)

AM Michael Van Canneyt wrote

If I understood the OP correct, he wants to change the use of "string" arguments in the public API to unicodestring. That changes a lot.

And that's why I asked here first.

(See e.g. https://bugs.freepascal.org/view.php?id=35113 for a similar situation where part of the error is that the lazarus user must explicitly call Utf8Decode.)

In Lazarus (with UTF8hack) assigning a UnicodeString to an AnsiString will call Utf16ToUtf8 on the implicit conversion.
So, explicitely calling Utf8Encode should not be necessary.

So my proposal is to leave the public API as-is, using string, adding

This leaves my initial "itch": input strings are CP_ACP (so can be anything), output strings are CP_UTF8 always.
Why do we convert:
SomeUnicodeString := SomeAnsiString (implicit conversion using WideStringManager)
but
SomeAnsiString := Utf8Encode(SomeUniCodeString) (explicit conversion bypassing WideStringManager)

IMHO this is rather inconsistent and it makes no sense from the viewpoint of "pure" freepascal users.
(Again: Lazarus users don't care one way or the other.)

E.g. compare that to FindFirst with AnsiString: it implicitely converts Ansi- to UnicodeString and lets the WidestringManager handle the conversion back to AnsiString.

unicode string overloads where possible/useful.

That would mean overloading almost all methods.

Bart

PM Michael Van Canneyt wrote

But inner workings can be made to use Unicode, because the underlying APIs are using unicode: The *W registry calls on windows, XML DOM on other systems.

Well, my argument is that since we interface explicitely with a UnicodeString API, then why not expose that to the programmer?

Note: TRegistry uses only on Windows a UTF-16 API. On non Windows systems it uses TXmlRegistry, which uses AnsiString and internally uses TXMLDocument, which uses UnicodeString.

Perhaps it would be better to change TXmlRegistry to Unicodetring?

Mattias

Op 2019-02-26 om 19:27 schreef Mattias Gaertner via fpc-devel:

Perhaps it would be better to change TXmlRegistry to Unicodetring?

I think that would be best, yes.

Marco van de Voort wrote

Op 2019-02-26 om 19:27 schreef Mattias Gaertner via fpc-devel: > Perhaps it would be better to change TXmlRegistry to Unicodetring? I think that would be best, yes.

That was what I meant with 'switch the internals to unicodestring'

Michael.

Bart wrote

This leaves my initial "itch": input strings are CP_ACP (so can be anything), output strings are CP_UTF8 always.

Unless your DefaultSystemCodePage is CP_UTF8 (in which case UTF8Encode is not needed), the next action on the string will convert away from UTF8 anyways.

String codepages are not stable, there is almost never a point in explicitly setting one...

Exactly.
It also breaks backwards compatibility.
Strings returned were encode in the systems codepage up to 3.0.4, now thy are not.
There will be programs out there that access individual chars of those returned string parameters, and they will now fail for any character that is not lower ascii (7-bit ascii).

Also it makes no sense to return strings in UTF8 and not require input strings to be encoded the same.
The argument that reading a string is now unicode proof also makes no sense to me, since, unless your systemcodepage is UTF8, there is no way to write a unicode character back to registry. If we need that, we need overloaded methods with either Utf8String or UnicodeString parameters.

I don't mind leaving the registry functions using plain ansistrings, but then let go of the idea it could support Unicode if your systemcodepage is not UTF8.

B.t.w. enforcing UTF8 on the results rather complicates the code for returning a TStringList.

Op 01-03-19 om 22:19 schreef Bart:

The utf8encode should go, just like the utf8decode's that we fixed already.

Regards,

Joost.

PM Joost van der Sluis wrote

The utf8encode should go, just like the utf8decode's that we fixed already.

Shall I post a patch in the bugtracker?
If so: instead of
SomeAnsiString := UTf8Encode(SomeUnicodeString) do I make it an implicit conversion (SomeAnsiString := SomeUnicodeString), or an explicit typecast (suppressing warnings: SomeAnsiString = String(SomeUnicodeString))?

As a side note: I now have several patches for the registry in the bugtracker, and it gets increasingly difficult to make new patches for each issue if the "previous" (as in: the ones I made earlier) don't get applied.

Bart

PM Bart wrote

As a side note: I now have several patches for the registry in the bugtracker, and it gets increasingly difficult to make new patches for each issue if the "previous" (as in: the ones I made earlier) don't get applied.

Applying the patch for this should if possible be done after applying the patch from https://bugs.freepascal.org/view.php?id=35022 (that patch references a line with Utf8Encode still in it, so it possibly can't be applied anymore if that line no longer exists).

This is good if you assign the result of ReadString() to a regular ansistring var. But if you assign it to a utf8string,unicodestring,widestring var - it will not return correct result for chars that are missing in the current system code page. This is bad for a user app which is using unicodestring everywhere and don't bother with ansistrings and their default code page.

The proper solution is to make string params and the result to be utf8string. This way proper conversion to regular ansistring will be done for the result and unicodestring/widestring will be properly handled as params.

The only drawback is when you pass constant string as parameter, you need to explicitly specify the code page of the source file. Otherwise national chars will be lost.

Yuriy.

AM Yuriy Sydorov wrote

This is good if you assign the result of ReadString() to a regular ansistring var. But if you assign it to a utf8string,unicodestring,widestring var - it will not return correct result for chars that are missing in the current system code page. This is bad for a user app which is using unicodestring everywhere and don't bother with ansistrings and their default code page.

Did you actually read this entire thread?

Yes. And I've made some tests and came to the conclusion above. In my opinion it is the most compatible approach.

Yury.

Op 3/3/2019 om 11:17 AM schreef Yuriy Sydorov:

(which is the case here)

Yes, but this is a feature of the language. String conversions go over the default stringtype, and if that can't harbor all characters, there will be loss.

The proper solution is to make string params and the result to be utf8string. This way proper conversion to regular ansistring will be done for the result and unicodestring/widestring will be properly handled as params.

We already have two and an half options:

- Delphi 2009+  everything unicodestring

- Delphi old      everything locale windows encoding

- Lazarus hack    set default encoding to utf8.

On *nix the last two are usually the same, and in time, the last two might also merge on windows, since Windows has options to set the default codepage to utf8 since 2018 april update (though I have not seen testreports yet).

There is no need to further change interfaces to provide yet another option which does yet another option which is useful for only a limited time.

PM Marco van de Voort wrote

There is no need to further change interfaces to provide yet another option which does yet another option which is useful for only a limited time.

So, where to go now from here?

1. Leave all method signatures unchanged (use plain string variables) and cut out all the Utf8Encode(). Then wait for the time that string=unicodestring?
2. Cut out Utf8Encode(), overload all methods with ansistring parameters to accept either UTF8String or UnicodeString parameters?
3. Do nothing at all?

Option 1:
+ easiest fix.
+ consistent with most other "interfaces" (for lack of a beter word).
+ no data los with Lazarus unicode hack. No dataloss on any system that has CP_UTF8 as it's sytem codepage (most *nix).
+ at least one person is willing to provide such a solution.
- possible data loss on Windows in plain pascal programs.

Option 2:
+ solves the issue of possible data loss cross-platform
+ makes no difference for Lazarus users.
+ at least one person is willing to provide such a solution.
- clutters the interface
- when string becomes unicodestring (like Delphi) (if ever?), the ansistring methods need to be removed (or parameters must explicitely typed as AnsiString)

Option 3:
+ no change required
- no problem solved
- leaves inconsistent interface

From my previous posts on this topic (here and in bugtracker) it will

be clear that I am strongly opposed to solution 3.

We should at least do something.
As it is now, the registry has bugs and regressions (as compared to 3.0.4) If nothing is done, then most likely Lazarus will fork the TRegistry to at least be able to fix current issues.
This is a situiation that should be avoided IMHO.

A decision needs to be made...

Op 3/6/2019 om 12:48 PM schreef Bart:

1+2.  Reverse manual enforced encoding, and honour the codepage in the default string type.

If unicode MUST be supported on windows without lazarus hack, convert the interface to unicodestring. (or maybe overload, but I would just convert it all)

There is no utf8string usage currently on Windows, simply because it will only trigger request for more manual encoding checks somewhere else because on any mixed encoding operation (including literals?), it still gets converted to default string as intermediate type.

PM Marco van de Voort wrote

1+2. Reverse manual enforced encoding, and honour the codepage in the default string type.

(2 sort of implies 1 as well...)

If unicode MUST be supported on windows without lazarus hack, convert the interface to unicodestring. (or maybe overload, but I would just convert it all)

Well, convert to unicodestring was my initial proposol, but this was dismissed in this discussion.
Overloading may make all users (except the maintainers of this code) happy?

Please add overloading using UTF8String.
Backwards compatibility is not an idle word.

Michael.

PM Michael Van Canneyt wrote

Please add overloading using UTF8String. Backwards compatibility is not an idle word.

To be honest, that last sentence does not make sense to me. Before trunk TRegistry used (on Windows) the A-API and string parameters are just AnsiString(CP_ACP).
Unicode caharcters were not supported in plain fpc programs using the default 1 byte encoded string type on Windows.

Introducing Utf8Encode (and the plain wrong Utf8Decode) in TRegistry is backwards incompatible.
Reverting this to using plain String does not introduce any backwards (compared to 3.0.4) incompatibilty AFAICS.
(Unicode support in plain fpc programs simply is not there (unless you use the appropriate string type). There is a reason for the Lazarus UTF8-hack.)
Introducing UnicodeString overloads does neither introduce regressions AFAICS. If it would have worked with A-API it certainly will work with UnicodeString, because the characters would have been inside the users codepage.
It does however add unicode support for plain fpc programmers for those who need that.
All would still be completely unicode-proof in Lazarus (with UTF8-hack).

Using UTF8String would trigger 3 conversions instead of 1 (plain fpc programs): string->utf8string->unicodestring (the last conversion is inside the methods where we call the W-API).
Using UnicodeString would trigger only 1 conversion: String->UnicodeString. For users using Utf8String inside there program calling the UnicodeString overload would also be just 1 conversion (lossless).

So, what do you propose for maximum backwards compatibility ?

By this I mean, for people that basically use plain strings and ASCII, they should not get warnings about
'conversion from widestring to ansistring' and vice versa.

Michael.

PM Marco van de Voort wrote

If unicode MUST be supported on windows without lazarus hack, convert the interface to unicodestring. (or maybe overload, but I would just convert it all)

And then there is of course ReadStringList/WriteStringList. Unless your systemcodepage is UTF8, dataloss will occur with characters outside your codepage.
There does not seem to be a proper solution for that without enforcing a UTF8 encoding on the individual strings (you cannot simply set Text property for that).
AFAIK we don't have a TUnicodeStringList?

In hindside, with this Unicode problem in mind, using a TStringArray would have been better.
AFAIK Delphi does not implement a ReadStringList/WriteStringList.

I see.
If I have in unit registry:

procedure TRegistry.A(S: String);
var
  u: UnicodeString;
begin
   u := S;
   A(u);
end;
procedure TRegistry.A(U: UniCodeString);
begin
  ...
end;

and then in my program:

var
  S: String;
  R: TRegistry;
..
  R.A(S);

And given that the procedures A are part of fpc distribution, and the path to the source is NOT in your search path for the compiler (i.o.w. just a regular fpc user, not someone who builds fpc) then the end-user (the programmer) would not see that warning if I'm correct?

When building fpc I get tons of conversion warnings that I don't get when I simply build my program.

Yes.

When building fpc I get tons of conversion warnings that I don't get when I simply build my program.

Ideally, they should all be fixed. They are all potential sources of error.

Michael.

PM Michael Van Canneyt wrote

Ideally, they should all be fixed. They are all potential sources of error.

You can't have it both ways: use CP_ACP string and expect them to be able to handle Unicode outside of their codepage, unless your codepage is UTF8.

Not sure I follow you ?

If somewhere there is a warning about conversion of unicodestring to ansistring (often abused as single-byte string) then this must be looked at and somehow fixed.

This can mean changing the single-byte string type to UTF8String and doing a UTF8Decode/Encode. Needs to be checked on a case-by case basis.

Michael.

Initially I replied to you saying it (have overloads with plain string and unicodestring) wasn't backwards compatible.
I then tried to point out to you that the past (the back in backwards compatibilty) was that string was a single byte enocded type and on Windows each character was just one byte and since there are only 256 possible characters this way codepages exists and you cannot represent e.g. a Russian character in a Wester European codepage. The main point here being that each singe byte represented a character.

Now Lazarus from the start has treated AnsiString as if it were UTF8. For people who are not used to this, this is a really big difference.

Using UTF8String in TRegistry instead of String forces users to consider the fact that returned strings are Utf8Encoded now always, even if they (probably most of them) do not need that because what they retrieve from and put in the registry fits into their codepage. And somebody out there will have code that checks if ReturnedString[Index] = #$E4 ("ä" in my codepage), and have the sourcefile in ANSI (system codepage), and now this will fail, because that character will now be made up of 2 bytes.

All that lead me to say that you cannot have Unicode in (classic) 1-byte encoded, (using default system codepage) strings.

Using UnicodeString/String overloads means that "old style" programs still function as the did before and anyone who needs it can use the UnicdeString variant directly.
And if you system codepage happened to be UTF8 in the first place, you don't care either way.

Anway, i am starting to repeat my previous arguments. So either I am unable to make myself clear, or I'm just plain wrong.

If you declare a function result as utf8string instead of string (ansistring) then automatic conversion will be performed when you assign the result of the function to a variable of type string (ansistring). You will gen a classic 1-byte per character string if your current encoding is 1-byte encoding. I mentioned this earlier.

Just check this. Change the declaration to:

function TRegistry.ReadString(const Name: utf8string): utf8string;

Yuriy.

PM Yuriy Sydorov wrote

If you declare a function result as utf8string instead of string (ansistring) then automatic conversion will be performed when you assign the result of the function to a variable of type string (ansistring). You will gen a classic 1-byte per character string if your current encoding is 1-byte encoding. I mentioned this earlier.

I know that, but you do not need to assign the functionresult to another string to investigate it.
Stupid example:
program test;

function x: utf8string;
var
  u: unicodestring;
begin
  setlength(u,3);
  word(u[1]) := $E4; //my editor is UTF8 so therefore this workaround
instead of u := 'äëï';
  word(u[2]) := $EB;
  word(u[3]) := $EF;
  result := utf8encode(u); //äëï but now Utf8Encoded
end;
var
  u8: utf8string;
begin
  u8 := x;
  if byte(u8[1]) = $E4 then writeln('OK') else writeln('Fail');
end.

It prints Fail, where it would have printed OK if x would have returned string.

This a corner case, but it definitely is a regression nevertheless.

Of course if "u8" is utf8string, then then first char will be encoded as a 2-byte pair. But if you change "u8" to be just "string" or "ansistring", then the first byte would contain "ä" if the current ansi code page supports it (eg cp1252). It is perfectly backward compatible.

Yuriy.

PM Yuriy Sydorov wrote

Of course if "u8" is utf8string, then then first char will be encoded as a 2-byte pair. But if you change "u8" to be just "string" or "ansistring", then the first byte would contain "ä" if the current ansi code page supports it (eg cp1252). It is perfectly backward compatible.

Sorry, wrong example.
Querying the result without assigning it to a variable will fail (I tried to say that in my previous post):

program test;
function x: utf8string;
var
  u: unicodestring;
begin
  setlength(u,3);
  //00E4 00EB 00EF
  word(u[1]) := $E4;
  word(u[2]) := $EB;
  word(u[3]) := $EF;
  result := utf8encode(u);
end;
begin
  if byte(x[1]) = $E4 then writeln('OK') else writeln('Fail');
end.

This would have written OK if x (e,g, TRegistry.ReadString()) returned string instead of UTF8String.

I posted a patch in https://bugs.freepascal.org/view.php?id=35213. Again: please first aplply the patch from
https://bugs.freepascal.org/view.php?id=35022 first, since that patch references code that has UTF8Encode in it.

No.

Please, a single patch with full solution for unicode & ansistring.

It becomes increasingly difficult to estimate the consequences of all this.

Michael.

No. Please, a single patch with full solution for unicode & ansistring.

You misundersdtand me here, I think.
The patch I mentioned does not concern the Unicode patch file however has a chunk of unaltered code in it, and in _that_ code there is one Utf8Encode.

If the patch to remove Utf8Encode
(https://bugs.freepascal.org/view.php?id=35213) is applied before the patch in https://bugs.freepascal.org/view.php?id=35022 , that latter one becomes invalid, since it references a code-block that is no longer there.

If however, OTOH, you do no want to apply the patch from https://bugs.freepascal.org/view.php?id=35213 because you want an "all containing" patch for that issue, I am a bit puzzled:
1. The proposed patch resolves a regression. This in itself is IMHO a reason to apply it as is.
2. Joost applied the patch to remove Utf8Decode.
3. From what I get from this discussion, there still is no communis opinio on how to implement full unicode support in TRegistry (use Utf8String or UnicodeString overloads for the TRegistry methods).
4. IMO breaking up this process in steps, will make it easier to see why a particular line of code was changed. (The "full solution" seems to be come a rather large and complicated diff.

I have already written tests for this issue.
I am willing to write more.

It becomes increasingly difficult to estimate the consequences of all this.

For me it becomes increasingly difficult to work on the patches for the various TRegistry bugs in the bugtracker if none of the patches there get applied.
This is frustrating since currently there seem to be only 2 persons who are prepared to write fixes for TRegistry (Serge Anvarov and me).

I get the feeling this subject is just not "sexy" enough.

That is indeed a misunderstanding.

It does not resolve a regression.

The current behaviour is a bugfix for a reported bug.

Your patch to undo it will maybe resolve one situation, but reintroduces the other bug. So instead of you, someone else will now be annoyed...

2. Joost applied the patch to remove Utf8Decode.

Well, if Joost wants to take care of it, great:
I have unassigned all TRegistry related bugs I assigned to myself.

I don't ever use the registry, so in that sense you are right it is indeed not sexy enough to worry about it :-)

Michael.

If I may add my two pence...

I'm all for speed and efficiency in library code, but conformance is more important - if a correct input (even if somewhat extreme) results in a bad output, then it's something that should be fixed.  If you try to feed the unit malformed input and it predictably produces bad output, it's probably permissible to leave it as is, since you're trying to break it.  To use your example of UTF-8, it's perfectly reasonable to assume that Utf8Decode is expecting valid UTF-8 data and attempt to decode it.  If you absolutely have to determine if the string is valid UTF-8, then you should call another function to determine if that is the case, and decide yourself how to handle the situation if the string is invalid.

The only time you should include sanity checks is in order to prevent exceptions (e.g. reading beyond the end of the string even though a UTF-8 lead byte implies there's more data), but even then there are situations where these could be allowed (e.g. if a subroutine takes a pointer as a parameter, then it can be assumed that it isn't null).

Gareth aka. Kit

On Sun 10/03/19 22:40 , "Bart" sent:

PM Michael Van Canneyt wrote

I don't ever use the registry, so in that sense you are right it is

indeed

not sexy enough to worry about it :-)

Neither do I use it...

Long time ago I wrote a patch for some other part of the RTL. The old behaviour was plain wrong and resulted in wrong data. When I wrote a patch that fixed the issue (string conversion into some other data type), I was told was that the old implementation was faster and therefor better.

I'm kind of feeling the same way now.

PM Michael Van Canneyt wrote

It does not resolve a regression.

Yes, it does and I showed it here.

The current behaviour is a bugfix for a reported bug.

And it did that the wrong way.
Calling Utf8Decode on a string without checking it is indeed Utf8 encoded is plain and simply wrong.

I may be totally wrong here, but form the discussion with you I get that you find the String <-> UnicodeString conversions are wrong, because they can cause data loss with strings not encode as UTF8. On the other hand you argue that "The use case of a non-lazarus command-line application is so minor that this can wait for a proper solution for unicode."
The combination of these arguments does not make sense to me. Lazarus programs will not suffer from dataloss in the String<->UnicodeString conversions, because all strings in Lazarus are UTF8 encoded.
The sample program in the bugreport that lead to this mess (https://bugs.freepascal.org/view.php?id=32185), would also have been solved without the Utf8Decode/Utf8Encode (the sample program is a Lazarus GUI program, so it includes the Lazarus Utf8-hack).

AFAICS cutting out the Utf8Decode/Utf8Encode fixes a regression and does not introduce a new regression: all this when compared to behaviour in 3.0.4.

The net result of all of this discussion is that I am getting frustrated.

PM Michael Van Canneyt wrote

I don't ever use the registry, so in that sense you are right it is indeed not sexy enough to worry about it :-)

Neither do I use it...

Long time ago I wrote a patch for some other part of the RTL. The old behaviour was plain wrong and resulted in wrong data. When I wrote a patch that fixed the issue (string conversion into some other data type), I was told was that the old implementation was faster and therefor better.

I'm kind of feeling the same way now.

What's the problem ?

I explained my view on how the TRegistry problems should be solved. I will not repeat it.
For organizational reasons, I personally prefer a single patch that implements what I consider correct, and not a series of incremental patches each addressing part of the problem.
I mentioned that in a comment on one of the bugreports.

Apparently Joost applied a patch addressing part of the problem. That's also fine: he does the work so he decides the how. That has always been the rule in FPC.
(Not sure whether he can now also close the associated bugreport ?)

So, please carry on. Someone will eventually apply the patches!

Michael.

AM Michael Van Canneyt wrote

What's the problem ?

The problem is that you make me feel that evreything I do or think of is wrong. Most likely this would not be the case if we were in the same room and talked about it.

Another practical problem is that we (as in the participants in this thread) still have not decided which way to go with this. Do we want overloads for all public/protected TRegistry methods that use UnicodeString or Utf8String, or both, or none of that?

For organizational reasons, I personally prefer a single patch that implements what I consider correct, and not a series of incremental patches each addressing part of the problem.

I understand that, but ATM it is almos undo-able for me to work on the registry. Every time I work on one issue I have to revert all chages I made for other issues.
This becomes very cumbersome, very fast.
If I start writing the "all including" unicode patch for TRegistry it will include unrelated fixes, otherwise I cannot test what I'm doing.

IMO, since this is trunk, patches need not be perfect in order to be apllied, they just need to be better than the old situation. Of course judging that is up to the devels.
Apply and then gradualy better the code based on feedback.

Once "good enough" merge all fixes to 3.2 branch.

Another problem for me is the not-windows part of the registry. E.g. TRegIniFile in the XML implementation is totally broken for at least 6 years.
Nobody has posted a bugreport about that in that period.

Now that comes to bite me in the *** when I fix some bugs in the windows implementation of TRegIniFile.
Fixing the XML implementation of that is currently beyond my capabilities. (In my personal opinion I'ld get rid of the not-windows implementation of TRegInifile. Yes it is backwards incompatible, but there cannot be users out there using it, otherwise bugreports would have been filed.)

So, please carry on. Someone will eventually apply the patches!

I will try to.

B.t.w. Michael: you unassigned yourself from
https://bugs.freepascal.org/view.php?id=35213 and
https://bugs.freepascal.org/view.php?id=35022 , but you forgot to change status to either new, acknowledged or confirmed. Now the issues are still "blue" in the bugtracker, so every other devel will think the issue is assigned to some other devel.
Can you please adjust the status of the issues?

Sorry to hear this.
That was certainly not my intention, on the contrary: I am glad to see that someone takes the time to look at some of these bugs in earnest.

Most likely this would not be the case if we were in the same room and talked about it.

I share your point of view on this :)

There I would opt for overloads that accept unicodestring.

People that need unicode use those, people that use ASCII (or ANSI) use the 'old' ones.

The current solution of trying to squeeze Unicode in UTF8 apparently creates more problems (or at least discussion) than it solves.

Fine with me, but I cannot speak for the other devs.

As long as it results in a working TRegistry class. Just clearly mark what bugs it is supposed to fix.

There are tests for TRegistry checked in. If they don't complain, all fine...

This is probably the approach the other devs would like to see :)

Probably because no-one in his right mind uses TRegIniFile ?

TRegIniFile is a component from Delphi 1 or 2, to facilitate the transfer from ini -> registry which was relevant in 1995 or thereabouts.

Maybe we should simply ditch it, or move it to a separate unit and mark it deprecated.

See above.

I did so.

Michael.

PM Michael Van Canneyt wrote

Sorry to hear this. That was certainly not my intention, on the contrary: I am glad to see that someone takes the time to look at some of these bugs in earnest.

Thanks.

From my perspective: the public API is all that matters. There I would opt for overloads that accept unicodestring.

I can work on that.

Probably because no-one in his right mind uses TRegIniFile ?

+1

I agree.

> Can you please adjust the status of the issues? I did so.

Thanks.

PM Michael Van Canneyt wrote

Probably because no-one in his right mind uses TRegIniFile ?

Mind you, the reason I looked into it was that on the forum somone actually reported it not working as expected.
So clearly there are (left-minded?) people out there using it (on Windows that is).

Great, thank you.

To keep things simple: A unit reginifile ?

Michael.

The world never ceases to amaze me !

Well, maybe these people should be elucidated and shown the right track, instead of being allowed to blunder on in a direction which is clearly a dead end...

(and I consider that a truth for right- or left minded alike ;) )

Michael.

PM Michael Van Canneyt wrote

To keep things simple: A unit reginifile ?

In the future perhaps. Let's first do the Unicode stuff.

PM Michael Van Canneyt wrote

> I can work on that. Great, thank you.

Could you (or some other devel) change thus summary of https://bugs.freepascal.org/view.php?id=35213 from "TRegistry should not enforce UTF8 encoding on returned string variables." to something like "Unicode aware TRegistry"?

It'll be more clear then that all unicode related fixes will go into that report, resulting (hopefully) in a single patch.

And while you're at it: add a relationship to #0034876 ? (#0034876 could probably the be resolved as a duplicate of #0035213, once that one's title has changed.)

Done and done.

Michael.

PM Michael Van Canneyt wrote

Done and done.

Thanks.

I will report back here if I have something I think is worth applying.

In the mean time ignore patches added to that bugreport. I'll attach intermediate diff's there, just because it eases my work flow. (And as backups of my work in progress, because I recently suffered a catastrofic failure on my Windows machine, because I mansged to f**k up the registry beyond repair. I don't have a VM where I do all my testing.)

PM Michael Van Canneyt wrote

From my perspective: the public API is all that matters. There I would opt for overloads that accept unicodestring.

Done that.
That was the easy part.

Now:
procedure GetKeyNames(Strings: TStrings);
procedure GetValueNames(Strings: TStrings);
procedure ReadStringList(const Name: String; AList: TStrings);

AFAIK we do not have a TUnicodeStrings(List).
Currently GetKeyNames and GetValueNames explicitely Utf8Encode the unicode (Utf16) result for that methods.
Do I leave it like that?

For ReadStringList an optional parameter (like: ForceUTF8: Boolean=False) can be added to control this behaviour. Delphi AFAIK does not have such a method, so Delphi compatibility is not an issue here.

Opninions?

Todo's for me:
- the above
- Make TXMLRegistry use UnicodeString
- Things I forgot about
- TRegIniFile: IMO don't change the API

I think this is the best.

What you could do is

TUniCodestringArray = Array of UniCodeString;
Function GetKeyNames : TUniCodestringArray;
Function GetValueNames : TUniCodestringArray;

The TStringList versions can call these and do the conversion.

Agreed.

Michael.

If your computer have enough RAM and have support for hardware virtualization (motherboard, its firmware and CPU) then I would recommend that you set up a VM for testing. I would recommend to you to use VirtualBox.

AM Cyrax wrote

If your computer have enough RAM and have support for hardware virtualization (motherboard, its firmware and CPU) then I would recommend that you set up a VM for testing. I would recommend to you to use VirtualBox.

That is not really the issue.
My machine is Win10 on i5 with 8GB memory.
I run Mint in aVirtualBox VM (a bit sluggish, but do-able). I don't have a Windows installable medium with license that I can install into a VM.
And I'm not gonna buy it, nor am I gonna use some pitate copy.

Bart

PM Bart wrote

That was the easy part.

TXMLRegistry does not seem to support Int64.
Any reason for that?

If it's an oversight (support for that was only recently added for Windows) should I try and implement it as well in the unicode patch or implement it later?

Bart schrieb am Do., 14. März 2019, 19:34:

Support for 64-bit values in the registry was only added a few weeks ago, so I'd say that was an oversight.
If you implement it, then please as a separate patch, maybe also with a separate bug report.

Regards,
Sven

PM Sven Barth wrote

Support for 64-bit values in the registry was only added a few weeks ago, so I'd say that was an oversight. If you implement it, then please as a separate patch, maybe also with a separate bug report.

OK
https://bugs.freepascal.org/view.php?id=35227

You don't need to buy a new license for your Windows 10. Just use its evalution method for a while.

You can use Wine inside your Linux VM, too.

Or use some of these application virtualizers :
<url:https://en.wikipedia.org/wiki/Application_virtualization>

PM Bart wrote

That was the easy part.

Since all values written are now UnicodeString, WriteStringList in XMLREG implementation now writes a sequence of UnicodeChar representations to the registry. Previously it wrote AnsiChar's. Give a stringlist with values '1','2','3'' it now writes: <Value Name="StringList" Type="4">31000000320000003300</Value> Before it did
<Value Name="StringList" Type="4">3100320033000000</Value>

Getting it to write AnsiChars is a little bit tricky as fas as I can see right now.

Since this function did not exist in 3.0.4 I think this is acceptable? Especially since the previous implementation was bugged as well. (Released software should not be made with fpc trunk anyway IMO)

Trunk is free to break compatibility with itself as long as the changes are also merged to the still unreleased 3.2. If it would be released already it would be a different topic...

Regards,
Sven

That's a reliefe.
I already planned to implement WriteStringArray/ReadStringArray, because it better fits to the API we're interfacing with. WriteStringList/ReadStringList would then simply use WriteStringArray/ReadStringArray.
The datatype (Type="4") would have to be the same for both mehods.

PM Bart wrote

I'm currently involved in some TRegistry bugs and regressions.

As this turns out to be a massive patch, I would really appreciate if for the time being no other patches were applied to the registry (there are several in the bugtracker), since they will almost certainly conflict with my unicode patch.

No worries. We know you're working on it. When you finalize your patch, we'll test the other TRegistry related bugs as well when we apply it.

Michael.

AM Michael Van Canneyt wrote

No worries. We know you're working on it. When you finalize your patch, we'll test the other TRegistry related bugs as well when we apply it.

OK, fine!

I have already declared TUnicodeStringArray in the Registry unit and use it for Read/WriteStringList.

I was about to implement GetKeyNames/GetValueNames this way, but hit a snag. The type TUnicodeStringArray must then be know to the XMLReg unit as well, since these functions call TXMLRegistry functions (in non-windows implementation).
I can declare the type there as well, but for the compiler these will be 2 different types.

Also I need conversion from TStrings to/from TUnicodeStringArray more then once. I made them private methods of TRegistry, but I could use them in XMLReg unit as well.
It is not absolutely necessary, but would make for better code maintenance.

XMLReg could have Registry in it's uses clause (must be in Interface section). Registry has uses XMLReg in th eimplementation section.

I have some recollection that we rather would not do it that way, because the compiler does not like it very much?
The alternative would be to define TUnicodeStringArray either somewhere in RTL or in a new unit that both Registry and XMLReg unit use.
(And then probably re-declare it in at least TRegistry (type TUnicodeStringArray = NewUnit.TUnicodeStringArray), so you can use the type in your program.)

Which way to go?

In Delphi yes, but normally not in FPC. If the base type is the same, 2 array definitions will be assignment-compatible.

Michael.

Thanks for pointing that out.

I implemented it that way now.

I attached registry.unicode.part5.diff for review to https://bugs.freepascal.org/view.php?id=35213 .

===
All public and protected methods now default to UnicodeString. Overloaded versions with String parameters use implicit conversion to/from UnicodeString.

Introduced new methods that read/write TUnicodeStringArray parameters. All methods using TStrings now uses the methods using TUnicodeStringArray. (In XMLReg unit I did not remove the existing implementation using TStrings, because AFAIK this unit can be used stand alone, and it would therefore not be backwards compatible.)

ReadStringList/WriteStringList have optional parameter to control if the Strings are encodes as UTF8.
GetKeyNames and GetValueNames with TStrings parameter encode the resulting strings as UTF8.
(I did not include an optional paramter as for Read/WriteStringList, because then the signature is no longer Delphi compatible. If so desired this paramter can be added. It's a trivial change.)

As discussed: in XMLReg WriteStringList now writes UnicodeStrings to the reg.xml file, which the previous implementation (trunk only) did not. It was judged to be acceptable.

TXMLRegistry internally now uses UnicodeString everywhere (except for the Filename property and HexToBuf/BufToHex, where it seemed to make no sense to convert this to UnicodeString).
There are still warnigs about implicit UnicodeString to String conversions, but they are in codepaths that are not used by TRegistry.

I changed some existing $ifdefs to make it easier to use and test the XML implementation under Windows.
(Simplye defining XMLREG would not compile under Windows)

I added some conditional compilation to XMLReg unit for the same reason.

When reading Reg_Multi_SZ data, trailing zero's are deleted regardless of value of StringSizeIncludesNull, because (by Windows definition) this data cannot contain empty strings.

In the XMLReg implementations I did some Explicit UnicodeString to String conversions in places where the string in question can only contain '0'..'9','A'..'Z'.
It suppresses a warning and it is safe by definition in those places.

I had to fix TXmlRegistry.HexToBuf.
The fact that it worked before was by pure luck.
=====

It builds on Win32, Win64 and WinCe.
I can now read/write Unicode keys, names in both XMLREG and Windows implementation.
I attached a reg.xml with Unicode Keys and Values as an example. I tested reading and writing all the default types (string, multistring, integer, int64, float, date, time, datetime).

There are still several debug writeln statements (all commented out) in the source code, they shall be removed at some point in time of course.

The patch file is 2063 lines....

I would appreciate a review and feedback.
Especially function TRegistry.GetKeyNames: TUnicodeStringArray; and function TRegistry.GetValueNames: TUnicodeStringArray; in the Windows implementation: where I assign u:=lpName (u=UnicodeString, lpName=LPWSTR), maybe a UniqueString is needed??
It does not crash here, but that may also be pure luck. I propose to continu the discussion in the bugtracker then, unless there are fundamental objections agains my proposed patch.

Il 14/03/2019 18:32, Bart ha scritto:

You don't need to buy a licence, or to use a pirated copy to run your Windows in a VM.

You may run your legal copy of Windows on a VM just by using the raw disk or raw partition feature of VirtualBox.

What I've done is the following.

I have repartitioned the original disk in order to free enough space for a Linux install.
By installing Linux on the free space, the first result is to have a dual boot box.
Then, in Linux I have created my VM machines, and among them a Windows VM which just uses the Windows partitions of the disk. That way the physical disk is the same of the original install, and Windows doesn't complain about licensing issues.
You may find detailed instructions on how to do it in the VirtualBox Help, under Advanced topics->Advanced storage configuration. But basically that's done from Linux with just one command line, which in my case is:

VBoxManage internalcommands createrawvmdk -filename ~/VirtualBox\ VMs/Windows7/windows7.vmdk -rawdisk /dev/sda -partitions 1,2,3,5 -mbr win7.mbr

where the required MBR has been created from Linux with sort of:

dd if=/dev/sda of=win7.mbr count=1

if you want to get the main MBR, or

dd if=/dev/sda1 of=win7.mbr count=1

if you want to get the MBR from the Windows start partition.

If you select a "conservative" Linux distro (such as CentOs) you get a rock solid, bullet proof, main OS for any other task, you may do a lot of Windows testing by just using wine, and you may safely play with all of yours VM's, creating snapshots before doing dangerous things such as tampering with registry. But you keep the chance to run the "native" Windows in order to judge the real performance, and look.

Maybe, before doing all of that, a full backup of your disk with Clonezilla would be a wise precaution....

Giuliano

In the following link you can download official and legal windows virtual machines for any test.

https://developer.microsoft.com/en-us/microsoft-edge/tools/vms/

El dom., 17 mar. 2019 a las 20:03, Giuliano Colla (<

On Mon, Mar 18, 2019 at 8:08 AM Silvestre wrote:

Thanks for the info, now please back on topic.

PM Bart wrote

I attached registry.unicode.part5.diff for review to https://bugs.freepascal.org/view.php?id=35213 .

...

I would appreciate a review and feedback.

...

I propose to continu the discussion in the bugtracker then, unless there are fundamental objections agains my proposed patch.

Bump?

I will look at it tomorrow. It has been a busy week.

Michael.

AM Michael Van Canneyt wrote

> Bump? I will look at it tomorrow. It has been a busy week.

Thank you, it got a bit silent here...
I'll await comments in the bugtracker then.

PM Bart wrote

> I will look at it tomorrow. It has been a busy week.

Thanks for applying.
Thanks to all of you for your advice and patience.

Should the changes be documented at
http://wiki.lazarus.freepascal.org/FPC_New_Features_Trunk#Units ?

Bart

Bart schrieb am So., 24. März 2019, 23:49:

Would be good, yes. Are you going to do it?

Regards,
Sven

I'll have a
Bart

AM Bart wrote

I'll have a draft at http://wiki.lazarus.freepascal.org/FPC_New_Features_Trunk#Registry_unit