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

P3471R4 Standard library hardening #7703

Merged
merged 1 commit into from
Mar 15, 2025

Conversation

Eisenwave
Copy link
Contributor

@Eisenwave
Copy link
Contributor Author

@jwakely I have had some issues when creating this PR:

  • The new note for [string.view.access] looks as if it had a paragraph number, but it's most likely intended to be attached to the hardened precondition paragraph. I have elected to ignore the paragraph number.

  • The paper deletes "Two kinds of implementations are defined" in [intro.compliance] but the library has the same sort of wording in [compliance] (library intro). How to proceed? Delete redundant wording? Mirror changes?

  • [structure.specifications] in the paper explains "Hardened preconditions" simply with two bullets that begin with "When". This is not how we usually do things. My suggestion would be something like "If the implementation is hardened ([intro.compliance]), ..." for the first bullet, and "Otherwise, ..." for the second bullet.

  • [basic.contract.eval] is a dead reference at this point. It should be resolved after merging with contracts.

  • It was very annoying that the paper doesn't have a wording diff that follows the structure of the standard linearly. For example, the paper first has the wording diff for 23.7.3.6.3 [mdspan.mdspan.members], and then the diff for 27.7.3.6.2 [mdspan.mdspan.cons]. The only thing this accomplishes is to bully the editor, and I'm the editor, and I feel bullied!

@frederick-vs-ja
Copy link
Contributor

frederick-vs-ja commented Feb 26, 2025

  • The new note for [string.view.access] looks as if it had a paragraph number, but it's most likely intended to be attached to the hardened precondition paragraph. I have elected to ignore the paragraph number.

The old note was semantically connected to the preconditions paragraph but placed in a bad (IMO) place. I guess the intent wasn't attaching the note to the paragraph, but such attaching seems to be an editorial improvement to me.

  • The paper deletes "Two kinds of implementations are defined" in [intro.compliance] but the library has the same sort of wording in [compliance] (library intro). How to proceed? Delete redundant wording? Mirror changes?

I think it's safest to mirror changes.

Copy link
Contributor

@burblebee burblebee left a comment

Choose a reason for hiding this comment

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

The paper was applied correctly except for 2 missing edits, but there were other issues - see comments.

Comment on lines 828 to 840
A freestanding
implementation is one in which execution may take place without the benefit of
Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

\pnum
A \defnadj{freestanding}{implementation}
is one in which % ...
A \defnadj{hosted}{implementation}
supports % ...
An implementation is either a
hosted implementation or a freestanding implementation.

Copy link
Contributor Author

@Eisenwave Eisenwave Mar 9, 2025

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 a7a861a

Copy link
Member

@jensmaurer jensmaurer Mar 9, 2025

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".

Copy link
Contributor Author

@Eisenwave Eisenwave Mar 9, 2025

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?

Copy link
Member

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.

@Eisenwave Eisenwave force-pushed the motions-2025-02-lwg-15 branch from a7a861a to 9ec312e Compare March 9, 2025 05:50
@tkoeppe tkoeppe requested a review from burblebee March 15, 2025 16:22
@tkoeppe tkoeppe force-pushed the motions-2025-02-lwg-15 branch from 9ec312e to 594d960 Compare March 15, 2025 16:24
@tkoeppe tkoeppe force-pushed the motions-2025-02-lwg-15 branch from 594d960 to 96e1c68 Compare March 15, 2025 16:34
@tkoeppe tkoeppe dismissed burblebee’s stale review March 15, 2025 16:36

Let's address that separately after the motions.

@tkoeppe tkoeppe merged commit b37eb4c into cplusplus:main Mar 15, 2025
1 check passed
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.

[2025-02 LWG Motion 15] P3471R4 Standard Library Hardening P3471 R4 Standard Library Hardening
6 participants