Improve std.windows.registry and fix datetime unit test #276
Conversation
| } | ||
|
|
||
| useWfuncs = save_useWfuncs; |
There was a problem hiding this comment.
How about scope(exit) useWfunc = save_useWfuncs;
right after line 2098?
|
Improve code pointed by blackwhale, and add datetime.unittest fixes. |
|
The changes in std.datetime look good, though I'm going to have to go and make some changes later to make it work with wine (wine doesn't use the correct keys in its registry, which is why the code was the way that it was). That doesn't affect what you did though. It looks solid, and it's probably cleaner than I would have done, since it's using std.windows.registry. |
| { | ||
| template SelUni(Char) | ||
| { | ||
| static if (is(Char == char)) |
There was a problem hiding this comment.
My first inclination is to think that Unqual should be used on the tests in this template, but I don't know. Certainly, in general, this sort of check requires Unqual.
| /** | ||
| Represents the local system registry. | ||
| */ | ||
| abstract final class Registry |
There was a problem hiding this comment.
abstract final? Is this an attempt to make Registry unconstructable? Couldn't you just use @disable on the default constructor?
There was a problem hiding this comment.
Hmm, Is it better the final class with @disable this();?
OK. I'll fix it.
There was a problem hiding this comment.
abstract final is just plain weird. It works, but using @disable on the default constructor is the way that the language provides to make a type unconstructable, so that's what should be done IMHO. I don't think that it matters one way or another whether it's final (since without a valid constructor in the base class, you can't construct derived types anyway), but it certainly doesn't hurt.
There was a problem hiding this comment.
A class that has @disable this() is not constructable, but it does't forbid inheritance.
In some cases, the derived class will work like a namespace inherits base namespace.
Therefore final is need for Registry class, it is isolated namespace.
There was a problem hiding this comment.
Well, I'm not sure that being able to inherit from a class that's unconstructable is really an issue, but as I said, it doesn't hurt to use final, so feel free to do so. However, now that I think of it, Walter has pointed out in the past that there is extra overhead in having a class rather than a struct (since it still ends up creating a vtable and all that), so it would probably be better to make it a struct rather than a class, which solves the inheritance issue too. I should probably do the same to Clock in std.datetime too, but I keep forgetting. I'll have to try and remember to do that in my next set of revisions to std.datetime.
There was a problem hiding this comment.
so it would probably be better to make it a struct rather than a class, which solves the inheritance issue too
Unfortunately, @disable this() in struct S does not disable its instantiation. It allows an explicit initialization like S s = S.init;.
I think that is still better, final class with @disable this().
|
Wow. That's a lot of changes. |
|
You can see the effective changes using GitHub's compare view, rather than having to look at each commit individually: |
|
Thanks for your many points, jmdavis. |
|
Updated from the review comments. Thanks very well, @jmdavis! |
- Use std.utf.toUTF16z - Fix array operations - Add next parameter for exceptions constructors - Remove unused code - Put the brace on its own line - Fix conditional compilation with endian - Move default case to the last of a switch block - Add more @safe pure - Fix ddoc comments. - Change auto to immutable - Add enforceSucc, and use enforce family - Change uint to size_t around indexing
Improve std.windows.registry and fix datetime unit test
|
Merged. |
std.windows.registry changes:
Use *W functions when
std.__fileinit.useWfuncsequals totrue.This is core improvement of this pull request. Current std.windows.registry unittest is broken, because Unicode code points are sometimes lack in MBS environment depending on CodePage setting, and in Japanese locale the unittest for registry write/read is always breaking!
This change can fix the serious problem.
Support ddoc.
Old doc comment is not ddoc formatting.
See http://d-programming-language.org/phobos/std_windows_registry.html
std.datetime changes:
Use std.windows.registry for getting TimeZones.
*A functions were also used in std.datetime for getting TimeZone, but was is broken in Japanese locale, because
StdandDltKeys inSoftware\Microsoft\Windows NT\CurrentVersion\Time Zones\(TimeZone Name)have Japanese values, and getting them with *A function causedInvalid UTF sequenceerror.But, this fix introduce cycle dependency with current unittest system in Windows.
(datetime -> windows.registry -> exception -> array -> algorithm => random -> datetime)
-> ... dependency required by module features
=> required by module _unittest_s
To fix it, we need to merge Each unittest in Windows #280 first.