Skip to content

Conversation

denis-sh
Copy link
Contributor

Added Windows's altsep support to getExt, getName and join (bugfix, I think).
Some cosmetic changes.

denis-sh added 2 commits May 28, 2011 04:31
template Char type in private functions => dchar
@kyllingstad
Copy link
Contributor

Before you spend a lot more time on this, I should probably mention that I have written a new version of std.path which I intend to put up for review for inclusion in Phobos as soon as possible. It correctly handles both dir separators on Windows.

@andralex
Copy link
Member

andralex commented Jun 8, 2011

Lars, what do we do this? I think it's safe to approve it.

@andralex
Copy link
Member

andralex commented Jun 8, 2011

s/do this/do with this/

@kyllingstad
Copy link
Contributor

It's probably safe, but it may be that there isn't much point in doing so. Based on feedback I've gotten so far, I feel quite certain that my revamped std.path will be accepted in some form or other. It is ready for review now, and it should be next in line after the TempAlloc review is over. Hopefully it will be included with DMD 2.055.

At any rate, if we do approve this pull request, I just noticed two lines that should be fixed. I'll comment on them separately.


static assert(sep.length == 1 && altsep.length == 1);
private bool isSep(dchar ch) {
return (ch == sep[0]) | (ch == altsep[0]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a boolean or.

 return (ch == sep[0]) || (ch == altsep[0]);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency (you use || everywhere else) and clarity of intent (it is a boolean operation). I know the bitwise OR works, I just think it looks strange.

@andralex
Copy link
Member

Ping? Both '|' and '||' work here, but probably the use of '|' is a bit surprising. So let's change that to '||' and commit soon. Thanks!

@denis-sh
Copy link
Contributor Author

Pushed "ping accepted" commit denis-sh/phobos@d3c7cfd to denis-sh:patch-3. No idea where is it.

@andralex
Copy link
Member

Hm. Let me pull this now. No need to make the change in a subsequent pull request (I think we're okay either way). Thanks!

andralex added a commit that referenced this pull request Jun 15, 2011
Windows's altsep support in getExt, getName and join (bugfix, I think)
andralex added a commit that referenced this pull request Jun 15, 2011
Windows's altsep support in getExt, getName and join (bugfix, I think)
@andralex andralex merged commit 37763e3 into dlang:master Jun 15, 2011
skoppe pushed a commit to skoppe/phobos that referenced this pull request Apr 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants