- User Since
- Apr 20 2015, 1:34 PM (423 w, 2 d)
Thu, May 18
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.
Wed, May 17
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.
Tue, May 16
I added a few comments and suggested edits, but this is mostly looking good.
Fri, May 12
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.
Thu, May 11
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.
Wed, May 10
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.
Tue, May 9
Fri, May 5
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.
Wed, May 3
Tue, May 2
Thank you for catching that @HazardyKnusperkeks! I completely missed (somehow) that the changed code modified Expanded. I offered another suggestion.
May 2 2023
Looks good to me.
Apr 27 2023
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.
Apr 26 2023
Apr 25 2023
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).
Apr 24 2023
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.
Apr 20 2023
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.
Apr 19 2023
Yeah, possibly - any suggestions on where?
Thank you for the detailed response, @mstorsjo. I understand the motivation for the changes much better now.
Apr 18 2023
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.
Apr 14 2023
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.
Apr 13 2023
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.
Apr 12 2023
This looks great. I added comments for some minor issues.
Apr 4 2023
Thank you for remembering to follow up on this!
Mar 30 2023
Sorry for the delayed response. I managed to miss seeing this review request.
Mar 29 2023
Thank you, Mariya! Looks good to me!
Changes look good to me. Thanks, Sindhu!
Mar 27 2023
It's not in the range of any of the universal-character-names.
Mar 26 2023
@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).
Mar 22 2023
Mar 14 2023
Mar 7 2023
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.
Feb 24 2023
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.
Feb 21 2023
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.
Feb 17 2023
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).
Feb 15 2023
Feb 13 2023
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.
Feb 9 2023
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).
Feb 7 2023
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.
Feb 3 2023
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.
Feb 2 2023
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.
Feb 1 2023
Rebased. This now targets Clang 17.
CI failures are due to test time outs in unrelated tests.
Jan 31 2023
Rebase and test update to ignore any declarations present in the global namespace when char8_t support is not enabled.
Jan 26 2023
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.
Jan 25 2023
Jan 19 2023
Looks good to me.