Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fs.path.append] Fix examples to show correct results for Windows #2098

Merged
merged 1 commit into from Jun 12, 2019

Conversation

jwakely
Copy link
Member

@jwakely jwakely commented Jun 6, 2018

The examples for path::append show the wrong results for Windows, not matching the description of the function. This has been discussed with Billy O'Neal who confirmed the correct results.

@tkoeppe
Copy link
Contributor

tkoeppe commented Jun 8, 2018

Looks fine to me -- @hubert-reinterpretcast, any opinions?

@jwakely
Copy link
Member Author

jwakely commented Jun 8, 2018

I forgot to explain that where we say:

both of the paths path("//host")/"foo" and path("//host/")/"foo" equal "//host/foo".

This is true even if the separator is backslash not forward slash, because equality is defined in terms of element-wise equivalence, not equality of the native() strings. So the choice of separator doesn't matter, they are both still "equal" to "//host/foo" for Windows even though one of them is "//host\foo" and one is "//host/foo".

@jwakely
Copy link
Member Author

jwakely commented Jun 8, 2018

The comment "On Windows, backslashes replace slashes in the above yields" should be removed because it's only true for one of "the above yields" not both, so let's just keep the POSIX examples and Windows examples separate.

@hubert-reinterpretcast
Copy link
Contributor

Looks fine to me -- @hubert-reinterpretcast, any opinions?

Looks okay to me, but I could not make definitive statements with regards to the behaviour for Windows.

@tkoeppe tkoeppe added the after-motions Pull request is to be applied after the pending edits from WG21 straw polls have been applied. label Jun 8, 2018
@zygoloid
Copy link
Member

zygoloid commented Jul 2, 2018

I think this example needs more work: these "yields string-literal" examples seem like a bad expositionary device. operator/ on paths produce paths, not strings. If we mean "yields path("...")", then that is what we should write. If we mean "yields a path whose pathname in the (generic|native) format is "..."", then that's what we should write (with a shorthand).

(As is, I can't judge whether this change is correct, because the example is meaningless.)

@jwakely
Copy link
Member Author

jwakely commented Jul 2, 2018

I'm surprised nobody's mentioned the fact that this example for operator/= doesn't even call operator/=

Maybe we should just rip it out entirely, as I've lost the will to fix it.

@zygoloid zygoloid force-pushed the master branch 2 times, most recently from e3dbfe2 to 1a21a65 Compare July 7, 2018 23:19
@zygoloid zygoloid added the changes requested Changes to the wording or approach have been requested and not yet applied. label Oct 8, 2018
@zygoloid
Copy link
Member

zygoloid commented Oct 8, 2018

I'm fine with just removing the example.

@jensmaurer
Copy link
Member

@jwakely: Ping.

@jensmaurer jensmaurer removed the after-motions Pull request is to be applied after the pending edits from WG21 straw polls have been applied. label Dec 1, 2018
@tkoeppe
Copy link
Contributor

tkoeppe commented Mar 15, 2019

@BillyONeal: Hi -- would you perhaps like to take over this issue?

@BillyONeal
Copy link
Contributor

It looks like @jwakely already resolved this with this change, and the discussion about "well the entire example section isn't very good" is a different issue entirely. I think we should merge this, as it is an improvement to the spec, and then move discussions of "operator=/ examples use operator/" to a different, lower priority issue.

@jwakely
Copy link
Member Author

jwakely commented Mar 18, 2019

It's not just that it uses operator/, Richard's comment at #2098 (comment) hasn't been addressed either.

I agree that fixing the results is an improvement, even if the overall presentation is still nonsense.

@jwakely
Copy link
Member Author

jwakely commented Mar 18, 2019

I've rebased and pushed a revised commit, PTAL.

@jwakely jwakely force-pushed the fs.path.append branch 2 times, most recently from 388d396 to caab798 Compare March 18, 2019 20:33
@BillyONeal
Copy link
Contributor

It seems like that comment is also unrelated to this change. (Indeed, it also affects the non-Windows examples here)

@jwakely
Copy link
Member Author

jwakely commented Mar 20, 2019

Indeed. I've tried to address all the problems in the latest commit anyway.

@jensmaurer jensmaurer removed the changes requested Changes to the wording or approach have been requested and not yet applied. label May 28, 2019
source/iostreams.tex Outdated Show resolved Hide resolved
@jensmaurer
Copy link
Member

@jwakely, thanks for the updates. I've added a review comment.

Also correct the fact that the results were shown as string literals,
not paths, and that the examples for path::operator/=(const path&) were
actually using the non-member operator/(const path&, const path&)
instead.

Also line up the comments to the 32nd column.
@jensmaurer
Copy link
Member

@hubert-reinterpretcast, is this agreeable now, in your opinion?

@hubert-reinterpretcast
Copy link
Contributor

Looks fine to me.

@zygoloid zygoloid merged commit daa9a7d into cplusplus:master Jun 12, 2019
@jwakely jwakely deleted the fs.path.append branch June 12, 2019 19:58
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.

None yet

6 participants