std.path review: second update

KennyTM~ kennytm at gmail.com
Sat Jul 30 08:27:33 PDT 2011


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



More information about the Digitalmars-d mailing list