Skip to content

Conversation

@eisenwave
Copy link
Member

Fixes NB US 181-294 (C++26 CD).
Fixes https://github.com/cplusplus/nbballot/issues/869.

@eisenwave eisenwave added the ballot-comment Response to an NB or ISO comment on a ballot label Nov 2, 2025
@tkoeppe tkoeppe requested a review from jwakely November 2, 2025 20:05
@eisenwave
Copy link
Member Author

I've asked @mattkretz about it, and he likes the longer comments for the "XXX permute" subclauses more.

The direction for the initial version of the PR is to copy the subclause title as is. The default assumption should be that the subclause title is more authoritative.

Also, we're not really consistent about referencing

  • "basic_vec"
  • "vec"
  • "basic_vec and basic_mask"
  • none

I'd rather not innovate and just delete it all. We can always create prettier comments in a future version if that's a good use of our time.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 2, 2025

Yeah, I think there was some conflict between NB comments wanting longer headings, and our overall style of not repeating the class names in headings. I'll take a look later.

@eisenwave eisenwave changed the title [simd] Synchronize synopsis references with subclause headings [simd.expos], [simd.syn] Synchronize synopsis references with subclause headings Nov 2, 2025
@jensmaurer
Copy link
Member

The comments in the synopsis should exactly copy the subclause headings. Yes, those are short-ish. It looks like what this pull request attempts to do.

I don't think SIMD is so special to warrant something different here.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 3, 2025

Indeed, and the NB comments may not have taken the wider context into consideration, so I think it's fine to accept "that we can improve things", but improve them in our own way.

Copy link
Member

@jensmaurer jensmaurer left a comment

Choose a reason for hiding this comment

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

So, it looks like we should just go ahead with this.

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 3, 2025

@eisenwave Could you please check your commit message(s)? I think there's a blank line missing after the first line, and I've seen this in several of your commits now.

@eisenwave
Copy link
Member Author

@tkoeppe fixed. I wasn't even aware there's supposed to be a blank line because I've looked Jens' commits, and the GitHub web client doesn't render them (the difference only becomes visible in git log).

@tkoeppe
Copy link
Contributor

tkoeppe commented Nov 4, 2025

Thanks! I think this is some form of Git convention. (Emacs warns you if you have multiple lines and the second line isn't blank...)

@tkoeppe tkoeppe merged commit ad99d52 into cplusplus:main Nov 4, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ballot-comment Response to an NB or ISO comment on a ballot

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants