This looks good to me now. I added one comment with suggested alternatives for the new error message, but your call on whether you like them better then what you have.

Thanks @Manna, this looks good to me now!

Splitting this into two patches (one to do the renames, another to perform the other changes) would simplify review.

One last minor request and then I'm happy with this.

I'm not opposed to these changes, but please note that these changes mean is will no longer be possible to use ubsan to discover when these data members are used before having been assigned an appropriate value. That is only a concern when an appropriate value would differ from the member initializers added via this change. I haven't reviewed the changes in depth, so I don't know if there are any such cases for which such a concern is applicable.

I added a few comments and suggested edits, but this is mostly looking good.

This looks great. I added one comment seeking clarification when an encoding error is present. If the code is as intended, it might help to add a comment to explain what is happening.

I do wonder if we need two bfloat implementations, but for that I'll leave a comment on D149573.

Thanks for all the updates @codemzs! I'm going to go ahead and accept. But please wait a few days for recently subscribed folks to have a chance to comment before landing this.

Heads up @clang-vendors. This patch changes the names of many internal identifiers for enumerators, class data members, and functions (including virtual functions) that are related to support for the __bf16 type. The changes are roughly to replace "bfloat16" with "bf16" to make the association obvious and to avoid confusion when support for the C++ std::bfloat16_t type lands (and to make room for a potential matching _BFloat16 type in C in the future). There are no changes to ABI or code generation; mangling continues to use the same names even where those have "bfloat16" in them. These changes will likely cause compilation failures in downstream projects that will require (simple) identifier updates to resolve.

I was following the LLVM contribution guidelines to use git clang-format, but I understand the importance of maintaining existing code styles that may be altered by git-clang format.

This looks great, thank you for doing this!

Looks good. Thanks @Manna!

Looks good to me!

I reviewed about a third of this, but then stopped due to the __bf16 vs std::bfloat16_t naming issues. I think the existing names that use "bfloat16" to support the __bf16 type should be renamed, in a separate patch, and this patch rebased on top of it. We are sure to make mistakes if this confusing situation is not resolved.

Looks good. Thanks again for catching our oops @HazardyKnusperkeks!

This looks good with one exception; I think the changes for class SemaDiagnosticBuilder are not what was intended.

These changes look good to me and I agree with not making a change for the KnownHeaders case.

Thank you for catching that @HazardyKnusperkeks! I completely missed (somehow) that the changed code modified Expanded. I offered another suggestion.

Looks good to me.

These changes all look good to me. For the DeclaratorChunk case, I noted an additional removal of a comment that I think should be considered. Assuming I haven't misread the code, DeclaratorChunk does not look like a small value type (at least not any more).

This change appears to have negatively impacted some users that were dependent on the previous Preprocessing::enableIncrementalProcessing(true) behavior. See https://github.com/llvm/llvm-project/issues/62413.

Thank you @cor3ntin, that was an excellent suggestion; this is much more readable now! I updated the new code to use custom verify tags (I left the existing code alone).

Looks good to me, thanks @Manna!

I added a comment to skip the one where the element type is a simple pair. The rest look fine to me.

Thanks, Soumi! Looks good to me!

It does something somewhat reasonable at least; fopen(); fputwc() ... on Linux writes the wchars as plain ASCII, anything outside of the ASCII range seems to produce literal ? chars.

With the exception of the case involving the Policy class, these changes all look fine to me.

This set of changes looks good to me.

Yeah, possibly - any suggestions on where?

Thank you for the detailed response, @mstorsjo. I understand the motivation for the changes much better now.

After seeing this review, I filed a support case with Synopsys with a request to stop reporting AUTO_CAUSES_COPY issues for small objects of class types, at least in the absence of additional evidence that a reference would be preferred.

The changes all look good to me. If there is a relatively simple way to track making sure that copyright statement gets updated, then great; I don't think that is worth holding up these changes though.

Resigning to remove this old review from my review queue.

These changes all seem reasonable to me. Some of them look like clear wins (e.g., ParsedAttrInfo).

Sorry for the very delayed review, @andrew.w.kaylor; I managed not to see this request until now.

This doesn't seem like the right approach to me.

I added some minor suggested edits, but otherwise, I think this is fine to accept.

It adds an include of #include <__format/formatter_integral.h> which ends up including <locale> which has internal definitions of isupper/islower causing clang to complain.

Any suggestions on what would be the right fix here?

Adding Corentin for awareness.

Adding Corentin for awareness.

This looks great. I added comments for some minor issues.

Thank you for remembering to follow up on this!

Sorry for the delayed response. I managed to miss seeing this review request.

Thank you, Mariya! Looks good to me!

Changes look good to me. Thanks, Sindhu!

It's not in the range of any of the universal-character-names.

@cor3ntin, do you have any thoughts about this? CWG 1871 (Non-identifier characters in ud-suffix) is somewhat related. I think the changed behavior makes sense when -fdollars-in-identifiers is in effect (which is the default).

I reviewed more thoroughly and this looks great to me! I added a comment about a possible preexisting issue with wchar_t that it might make sense to address with this change.

The changes made (from what I've seen, I haven't reviewed every line) make sense to me. The amount of change does make me a bit nervous though.

In D143349#4143309, @dimztimz wrote:

I find your concerns completely unjustified.

That can be done as a separate patch after this one gets accepted. One has to think how to incorporate that testing framework into this one, there is no straightforward way. That takes time. This testsuite is pretty comprehensive on its own. We should massage this one until its ready to be merged, and after than larger changes can be done.

The changes look good to me. It took me a while to convince myself that the intended behavioral change is correct, but I eventually concluded that the changes match the intent in [locale.codecvt.virtuals]p5 (http://eel.is/c++draft/locale.codecvt#virtuals-5).

Just noting for the record; these changes address review comments from https://reviews.llvm.org/rG8bb98b5715479210d2eec4d72344c21cb82bf78a.

Thanks @cor3ntin, the additional changes look good!

@cor3ntin, thanks for making these changes.

The guidance from EWG this week and in the past was that we are always required to 'parse and diagnose appertainment' of standard attributes, but not to enable __has_cpp_attribute unless we actually 'do' something with it. I intend/suggest we add a condition to the CXX tag of 'is supported' with some sort of conditional for checking diagnostic and O level (or just straight 'false' in this case).

Thanks for the update, @ahatanak. Please note that myself and Corentin are both preoccupied with the WG21 meeting this week, so it might be a while before either of us is able to take a good look at your update.

Incidentally, it looks like that LSI copy (https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/TreeTransform.h#L13469-L13478) has not been needed since commit bf5fe2dbba0899bee4323f5eaa075acc43a18e2e (see https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDecl.cpp#L15435-L15438).

https://github.com/llvm/llvm-project/issues/60517 has been filed to backport this change for the LLVM 16 release.

I'm still don't understand what the problem is about cleaning up the lambda scope.

Sigh, I forgot to tag the commit with a link to this differential review. Closing manually.

Rebased. This now targets Clang 17.

CI failures are due to test time outs in unrelated tests.

Rebase and test update to ignore any declarations present in the global namespace when char8_t support is not enabled.

Thanks @ldionne, I'm heading out of town for the weekend, so I'll wait to push this until Monday.

I'm inclined toward option 1 for two reasons: 1) use of glibc internal details like this is inherently fragile, and 2) There is no need for the C++ library to expose the C23 char8_t related declarations when builtin char8_t support is disabled.

You'll need a rebase to get the pre-commit CI to work.

Looks good to me.