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.

Sign up for GitHub

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

Jump to bottom

[format.arg] Apply \exposid consistently #6007

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

[format.arg] Apply \exposid consistently #6007

wants to merge 6 commits into from

Conversation

AlisdairM
Copy link
Contributor

Consistently apply the \exposid macro when typesetting exposition-only private data members and typedef-name members in basic_format_arg and related parts.

Note that I confirmed the exposition-only intent against P0645R10 to ensure that the use of code font was a genuine misapplication of the original paper.

@AlisdairM
[format.arg] Apply \exposid consistently
3ca20fa 
Consistently apply the `\exposid` macro when typesetting exposition-only
private data members and typedef-name members in `basic_format_arg` and
related parts.

Note that I confirmed the exposition-only intent against P0645R10 to
ensure that the use of code font was a genuine misapplication of the
original paper.
jwakely
jwakely requested changes Nov 27, 2022
View reviewed changes
Copy link
Member

@jwakely jwakely left a comment

Choose a reason for hiding this comment

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

I think you missed one, but otherwise I agree with the change. For libstdc++ I used this instead of the "not exposition-y" char_type:

using _CharT = typename _Context::char_type;

source/utilities.tex Outdated Show resolved Hide resolved
JohelEGP
JohelEGP suggested changes Nov 27, 2022
View reviewed changes
source/utilities.tex Outdated Show resolved Hide resolved
source/utilities.tex Outdated Show resolved Hide resolved
AlisdairM and others added 4 commits November 27, 2022 20:13
@AlisdairM @jwakely
Update source/utilities.tex
e1fb4cc 
Co-authored-by: Jonathan Wakely <github@kayari.org>
@AlisdairM @JohelEGP
Update source/utilities.tex
17f4dc1 
Thanks.  I made a very mechanical edit, and was not sure if the `\tcode` was redundant.

Co-authored-by: Johel Ernesto Guerrero Peña <johelegp@gmail.com>
@AlisdairM @JohelEGP
Update source/utilities.tex
789e434 
Co-authored-by: Johel Ernesto Guerrero Peña <johelegp@gmail.com>
@AlisdairM
Catch some missed exposid macros
003238d 
This should catch all of the missing *char-type* exposid macros.

Additionally, add a trailing underscore to the exposition only
private data member, 'value', and consistently render it as
exposid as well.
JohelEGP
JohelEGP suggested changes Nov 27, 2022
View reviewed changes
source/utilities.tex Outdated Show resolved Hide resolved
@AlisdairM @JohelEGP
Update source/utilities.tex
8274109 
Co-authored-by: Johel Ernesto Guerrero Peña <johelegp@gmail.com>
jwakely
jwakely reviewed Nov 27, 2022
View reviewed changes
Copy link
Member

@jwakely jwakely left a comment

Choose a reason for hiding this comment

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

I don't think we have agreement on adding underscores to data member names, i.e. value vs value_. There is no value() member function that it could clash with, so value should be ok.

@JohelEGP
Copy link
Contributor

Underscores are discussed at #4702.

@jwakely
Copy link
Member

jwakely commented Nov 27, 2022

Underscores are discussed at #4702.

Indeed, and given the general dislike of the underscores and the strong objection, I don't think adding them here is progress. At the very least, controversial.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 27, 2022

That's right, the naming of exposition-only variables is a delicate matter that we should not change as a drive-by.

@AlisdairM
Copy link
Contributor Author

While I strongly agree with the distaste for trailing underscores, I think this case merits special attention. value is the only exposition only private data member in the format framework that does not have a trailing underscore, unless it has internal dashes for kebab case to indicate that it is an exposition-only member. See args_, begin_, data_, end_, format_, indexing_, next_arg_id_, num_args_, out_, ptr_, separator_, size_, underlying_, and even opening-bracket_ and closing-bracket_.

On the other hand, I find just str in basic_format_string (so I was wrong about value being the only example).

I suggest making value_, and possibly str_, consistent through this facility, and then tackling the whole issue if we want to deal with trailing underscores.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 28, 2022

I see, yes, the rest of the formatter spec already uses underscores, so changing these two does indeed seem like an improvement. Go for it.

@wg21bot wg21bot added the needs rebase The pull request needs a git rebase to resolve merge conflicts. label Jun 28, 2023
@AlisdairM AlisdairM mentioned this pull request Aug 1, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jwakely jwakely jwakely requested changes

@JohelEGP JohelEGP JohelEGP requested changes

Assignees
No one assigned
Labels
needs rebase The pull request needs a git rebase to resolve merge conflicts.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@AlisdairM @JohelEGP @jwakely @tkoeppe @wg21bot