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

[swappable.requirements] Merge example's comment to one line #1829

Merged
merged 1 commit into from Nov 21, 2017

Conversation

JohelEGP
Copy link
Contributor

This follows the style of the surrounding comments.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 20, 2017

I think I'd rather just get rid of the pointless line break, non?

@JohelEGP
Copy link
Contributor Author

Of the surrounding comments, if that was done to the one below, it would merge the comments. Although it can be done for the one in the fix and the one above it, the current style is most general. What do you think? Do we keep the one below and remove them from the two above.

@JohelEGP
Copy link
Contributor Author

  swap(t1, t2);     // OK: uses swappable conditions for lvalues of type \tcode{T}

Oh, you mean like that?

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 20, 2017

Yes.

@JohelEGP JohelEGP force-pushed the swap_reqs_example_comment_fix branch 2 times, most recently from d586d1d to 7bdb186 Compare November 20, 2017 12:33
@JohelEGP JohelEGP changed the title [swappable.requirements] Move example's brace after comment [swappable.requirements] Merge example's comment to one line Nov 20, 2017
@JohelEGP
Copy link
Contributor Author

Done.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 20, 2017

Thanks!

@jensmaurer
Copy link
Member

Looks good to me.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 20, 2017

Oh, wait, this would overrun the line, right? Can you render it and see what it looks like? I didn't realize how far out the comment is. If it doesn't fit, let's go back to the original proposal.

@JohelEGP
Copy link
Contributor Author

I'll try rendering it. I think I've never done it before. However, I thought it was fine by looking at the code, as the comment's font should be mostly shorter than the code's monospaced font.

@JohelEGP
Copy link
Contributor Author

1511185745
If it was to match the column of the other comments, it wouldn't fit, so I had moved it to the preferred 20th column.

Avoids having the comment over the closing brace.
@JohelEGP JohelEGP force-pushed the swap_reqs_example_comment_fix branch from 7bdb186 to 1c4fdf5 Compare November 20, 2017 14:05
@JohelEGP
Copy link
Contributor Author

Actually, I just assumed it wouldn't fit, but it does. Now I know I should render in these cases.
1511186699

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 20, 2017

Oh, does it actually fit? Great.

@jensmaurer
Copy link
Member

I just tried it out locally; the change does not cause an overfull hbox.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 20, 2017

@jensmaurer: That's not relevant, though: what matters is that there isn't a line break. Long comments will happily go on as many lines as they need, all neatly justified. So the important check here was that it fits on one line. But it seems to!

@tkoeppe tkoeppe merged commit 34efb74 into cplusplus:master Nov 21, 2017
@JohelEGP JohelEGP deleted the swap_reqs_example_comment_fix branch November 21, 2017 10:55
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

3 participants