Do not call `getcwd` (or other `base`) in `absolutePath` if not needed. #522

Merged
merged 1 commit into from May 3, 2012

2 participants

@denis-sh

No description provided.

@kyllingstad kyllingstad commented on the diff May 3, 2012
std/path.d
// TODO: @safe (BUG 6405) pure (because of buildPath())
{
if (path.empty) return null;
if (isAbsolute(path)) return path;
- if (!isAbsolute(base)) throw new Exception("Base directory must be absolute");
- return buildPath(base, path);
+ immutable baseVar = base;
@kyllingstad
D Programming Language member

Why this line? Surely dmd has to be smart enough to only call getcwd the first time base is used?

@kyllingstad
D Programming Language member

Nevermind, I see now that it is supposed to be evaluated on every use, by design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kyllingstad
D Programming Language member

Would it make sense to do the same for relativePath, while we're at it?

@denis-sh

Would it make sense to do the same for relativePath, while we're at it?

Of course. Didn't notice that relativePath uses the same thing. Should I merge this two small commits to not pollute history?

@kyllingstad
D Programming Language member

I don't think that's necessary. (Personally, I prefer making many small commits rather than a few big ones, as long as each commit leaves the code in a working state.)

@kyllingstad
D Programming Language member

Hmm.. it seems there is a merge conflict, most likely due to a recent update I made to std.path. This means that the auto-tester is unable to test it. On which platforms have you run the unittests?

@denis-sh

Rebased.

On which platforms have you run the unittests?

I don't run any unittests because autotester made me lazy...

@kyllingstad
D Programming Language member

Cool, I wouldn't have expected GitHub to handle a rebase so nicely. All right, I'll merge the pull request when it has passed through the autotester.

@kyllingstad kyllingstad merged commit 2594c1a into dlang:master May 3, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment