-
Notifications
You must be signed in to change notification settings - Fork 769
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
P3471R4 Standard library hardening #7703
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7581,7 +7581,7 @@ | |
|
||
\begin{itemdescr} | ||
\pnum | ||
\expects | ||
\hardexpects | ||
\tcode{n < size()} is \tcode{true}. | ||
|
||
\pnum | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not your doing (from existing text), but wanted to point out that this is where the \defnadj for "freestanding implementation" should be (not above) since this is the definition. Same for "hosted implementation" in the sentence below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we want the
defnadj
to be on the first occurrence of the term.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I respectfully disagree - suppose the introduction of the term came paragraphs before the actual definition, yet in the same section? I'd say we always want the definition to be at the actual definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jwakely what if we fixed it up like this, which seems like it would make both of you happy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think this change is an absolute must anyway because [library] currently states whether an implementation is hosted or freestanding (it's impldef). This is way too late; core wording like [intro.multithread] already refers to freestanding.
If the
\impldef
for hardened implementations is in [intro], then so should the\impldef
for hosted vs. freestanding. This is also about mirroring changes and getting rid of the "there are two kinds of implementations" wording. See a7a861aThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These might all be good ideas, but (because changing existing, unchanged text) feel out-of-scope for the pull request that applies an approved paper from the motions. And it most certainly not a "fixup".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I still feel like it should be included here is that the paper intentionally removes "two kinds of implementation" wording. It seems like an obvious oversight that this wasn't also done in [library], and like something that this paper (and therefore this PR) should do. If we merged without the fix, then we'd need to fix up the knowingly incorrect state we've created later anyway.
Would it be fine to split up the commit that just mirrors the changes in [library] and append the remaining stylistic improvements as a non-fixup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When applying papers, the contents of the primary paper application commit should be what the paper says, plus punctuation and typo fixes.
If there are additional fixes that are editorial and directly triggered by the paper, they should be in a separate commit, with the usual "[label] title" description.
If, beyond that, there is a reason to clean up the wording even further, I'd prefer to have a separate editorial pull request that can be considered on its own merits. Just because the outcome of a paper application is a bit suboptimal doesn't mean we should rewrite the entire surrounding words; presumably, the applied paper was peer-reviewed by LWG and/or CWG.