Next in Review Queue: The New std.path

[Lars T. Kyllingstad]

On Sun, 17 Jul 2011 12:30:39 +0200, torhu wrote:

> On 15.07.2011 02:20, dsimcha wrote:
>> Lars Kyllingstad's new and improved std.path module for Phobos is the
>> next item up in the review queue.  I've volunteered to be the review
>> manager.  Barring any objections, the review starts now and ends at the
>> ends two weeks from now on July 28.  This will be followed by a week of
>> voting, ending August 4th.
>>
>> Code:
>> https://github.com/kyllingstad/phobos/blob/std-path/std/path.d
>>
>> Docs:
>> http://www.kyllingen.net/code/new-std-path/phobos-prerelease/
std_path.html
>>
>>
> Looks nice and clean, both docs and code!  I like modules that are not
> overengineered :)

Thanks!

 
> I noticed a couple of things that I don't think have been mentioned
> already:
> 
> The docs for defaultExtension say that there's one case where doesn't
> allocate.  But the code says that it always does (.idup isn't
> conditional, it's just .dup with a different return type).

There is no reason for idup to duplicate an array which is already 
immutable.  If it does, I'd say it is a compiler bug.
 

> joinPath also uses .idup, thus always allocates.  Maybe there could be a
> documented allocation policy for the module as a whole?  Copy-on-write
> used to be the general rule for Phobos, don't know if that's true
> anymore.  For most of the functions in this module, even a single heap
> allocation could be more expensive than the rest of what they do.

True.  I've tried keeping allocations to a minimum.  If you see other 
places where an allocation could be avoided (besides the idups), please 
let me know.

I'm not sure what you mean by copy-on-write in this case.  The functions 
in this module never modify the input arrays.


> I've read the discussions about the function names, but I still dare to
> make a suggestion of my own.  I feel like joinPath is a bit iffy,
> shouldn't it be plural somehow?  What it does is to create a path by
> putting pieces together, some paths in their own right, some not.  My
> suggestion is to simply call it buildPath.

Good point.  joinPaths would be another option.

-Lars