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