Andrei's std.path review

[Jonathan M Davis]

On Thursday, August 11, 2011 07:10:08 Lars T. Kyllingstad wrote:
> I am replying to this in a new thread, so as not to pollute the voting
> thread.  (Speaking from experience as a review manager, noise in the
> voting thread makes it harder to count votes.)
> 
> On Wed, 10 Aug 2011 12:37:01 -0600, Andrei Alexandrescu wrote:
> > * "use std.file.isDir and std.file.exists" -> use the XREF macro to
> > generate cross-reference links.
> 
> Cool, didn't know about that one.  Will fix.  Should it also be used to
> generate internal references (such as linking to filenameCharCmp in the
> filenameCmp documentation) or is there a separate macro for this?

XREF deals with linking to functions in other Phobos modules. CXREF deals with 
linking to functions in druntime modules. LREF deals with linking to functions 
within a module.

> > * filenameCharCmp and filenameCmp -> why long and not int?
> 
> filenameCharCmp() returns a-b, and since a and b are dchars, the
> corresponding signed type is long.  filenameCmp() returns long because
> filenameCharCmp() does.

I'd argue that you should just cast it to int and return int. All the various 
compare functions promise is whether the return value is less than, equal to, 
or greater than 0. Relying on the exact value is wrong. And normally such 
functions return int. So, I don't see any reason why these shouldn't be change 
to return int.

> > * Example in expandTilde uses odd ALL_UPERCASE variable names.
> 
> Ah, that would be an example from the old std.path.  Can't believe I
> didn't spot this before.  Will fix.
> 
> > Comments on the implementation:
> > 
> > * We're increasingly moving towards consolidating imports into one.
> 
> Seriously?  I haven't noticed this.  While I will certainly follow the
> Phobos conventions, it would be nice to know the rationale for this.  Is
> it simply for vertical compactness?  As you may have noticed, I'm not as
> worried about that as you are. ;)
> 
> Arguments in favour of separate imports are, in my opinion:
> 
>  - Adding/removing imports is easier.
>  - Seeing which modules are imported is easier.

A large portion of Phobos consolidates imports, but not all of it. Nothing 
I've been doing has been unless the module already did, since I much prefer 
separate imports, but I know that Andrei favors using one import statement. We 
don't really have an official rule on the matter though. I think that the only 
time that it's come up is when Andrei has mentioned it for one reason or 
another, which hasn't happened very often. I'm not sure what the opinions of 
any of the other Phobos devs are on the matter. Personally, I _much_ prefer 
separate imports per module.

- Jonathan M Davis