On Jul 30, 11 17:00, Lars T. Kyllingstad wrote:
On Sat, 30 Jul 2011 03:47:55 +0800, KennyTM~ wrote:
On Jul 30, 11 02:06, Lars T. Kyllingstad wrote:
[snip]
Thanks for the nice work!
Comments:
- pathSplitter: empty, front, back could be const. Also, make the struct
'static struct'.
Will do.
- hasDrive, isDriveRoot: the path
"#:\x"
should not pass hasDrive. In Windows only /[a-zA-Z]/ are supported drive
letters. '#:' may work but is not officially supported.
True, but then "#:" must be considered a directory name, which is not
much better. The module generally assumes that any path you pass to it
is well-formed. If not, the results are undefined.
The rationale for this is that you don't know whether a path is valid,
i.e. well-formed AND correct (pointing to the right place in the file
system), until you try to use it. std.path can in principle verify well-
formedness, but since it cannot verify correctness, both may just as well
be taken care of by the OS.
Fair enough.
- Bug 6390 (= 6337) has been fixed. Some CTFE tests can be enabled.
I know, but the fix hasn't been released yet. I'd prefer if people
didn't have to build the compiler from repo to test the module. I'll be
sure to enable the tests when DMD 2.055 has been released.
If this module is accepted, it will be released along with 2.055 which
carries the fix of 6337 also. Have to delay those tests to 2.056 doesn't
sound reasonable to me. It's OK if those are just disabled to ease
reviewing, but I'd argue that in the final commit, the tests must be
enabled whenever the trunk allows it.
- baseName, dirName "TODO: @safe pure nothrow": Mention which bugs are
preventing this (std.conv.to and std.string.chomp?).
Will do.
- buildPath, More than two path components: looks like a perfect
candidate for std.algorithm.reduce. And this should probably accept a
range, but I'm not sure.
I'll look into using reduce. I have considered letting the function take
a general range, but variadic parameters likely cover the vast majority
of use cases. If there is a real need for a range version, however, I'd
be happy to add it. Otherwise, I'd rather avoid the API clutter.
OK.
- CaseSensitive.osDefault: As I mentioned before, version(OSX) should
set this to 'no'.
Sorry, forgot about that. I'll fix it.
- globMatch: only UTF-8 is supported?
Except for the name change (fnmatch -> globMatch), this function is not
written by me. It is taken from the current std.path, and is for all
intents and purposes not part of my submission.
However, if it is trivial to redefine it in terms of general string
types, I will do so. I'll check.
OK.
- extension: Note that the DDoc text of 'extension' is highlighted.
Thanks!
-Lars
