...And now I'll do the honors and be the first to review. Overall this
looks solid and well documented but I have a few nitpicks and questions
1. Do we really need currentDirSymbol and parentDirSymbol? AFAIK
they're standard across all operating systems that anyone cares about
and everyone can easily remember them. I have never used the
equivalents in the old std.path and have never missed them.
2. There's really no need to always allocate a new array on things like
setExtension. You could just append the extension in the case where the
extension doesn't already exist and the string is immutable. I'm not
sure this is worth the extra complexity, though, given that setExtension
will probably never be used in perf-critical code.
3. All the stuff from the old std.path should be "scheduled for
deprecation". Deprecating stuff breaks people's build processes and
should only be done after adequate warning. I don't know a good way to
implement this for enums and aliases, though, which I guess begs the
question of whether DMD should support soft deprecation.
4. This module might be useful at compile time. As pointed out on this
forum, Phobos should eventually include tests for CTFE-ability. It
would be nice if std.path had a few CTFE unit tests sprinkled in.
On 7/14/2011 8:20 PM, 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
Author's comments -- please read before reviewing:
First of all, note that I have rewritten almost everything from scratch.
Mainly, this is because there are a lot of corner cases which aren't
handled correctly by the current std.path, and in the end rewriting
turned out to be easier than updating. (Check out the unittests; they
cover all the cases I could think of.) The only functions I have kept
unchanged from the current std.path are pathCharMatch (formerly
fncharmatch), glob (formerly fnmatch) and expandTilde.
Secondly, I hope you will find the new function names to be both more
descriptive, more consistent and more in line with current Phobos
conventions. They are the result of a (rather informal) round of
discussion and voting in this forum a few months ago (which I hope you
will consider before suggesting new names):
http://www.digitalmars.com/d/archives/digitalmars/D/Proposal_for_std.path_replacement_131121.html
Thirdly, I have applied @safe, pure and nothrow wherever possible.
There were cases where this was not possible, either due to bugs in DMD
(specifically, 5304, 5700 and 5798) or due to functions in other Phobos
modules not being properly marked with these attributes. I have marked
these with //TODO comments and will fix them as soon as possible.
And finally, due to the deprecated: block at the bottom of the module,
this version should be backwards compatible with the current std.path
for a while yet.
