[PATCH] D134475: [clang-cl] Add support for [[msvc::constexpr]] C++11 attribute

erichkeane added a comment. In D134475#4655362 , @RIscRIpt wrote: > Should I re-submit it to GitHub? While I hate that we'll lose all of the Phabriator history on this one, I don't see being able to complete this review in the next two weeks before

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

erichkeane added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3701 + QualType PointeeType = E->getType()->getPointeeType(); + if (PointeeType.isNull()) +return false; eddyz87 wrote: > erichkeane wrote: > > We override `operator bool` to make

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. 2 nits on the CFE, else LGTM! Obviously you still need an LLVM reviewer for the backend changes. Comment at: clang/lib/CodeGen/CGExpr.cpp:3701 + QualType

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

erichkeane added inline comments. Comment at: llvm/test/CodeGen/BPF/preserve-static-offset/load-align.ll:61 +!0 = !{i32 1, !"wchar_size", i32 4} +!1 = !{!"clang version 18.0.0 (/home/eddy/work/llvm-project/clang c899a1ca75d0f1b559204eff79a2578d2cafc7ab)"} +!2 = !{!3, !4, i64

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

erichkeane added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3700 +return false; + if (auto *BaseDecl = E->getType()->getPointeeType()->getAsRecordDecl()) +return hasBPFPreserveStaticOffset(BaseDecl); eddyz87 wrote: > erichkeane wrote: > >

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

erichkeane added a comment. In D133361#4652102 , @eddyz87 wrote: > Rebase, changes as requested by @aaron.ballman and @erichkeane. > > Hi @aaron.ballman, @erichkeane, > > Thank you for taking a look. > I beleive this commit covers all feedback except

[PATCH] D147844: [clang][Sema]Print diagnostic warning about precedence when integer expression is used without parentheses in an conditional operator expression

erichkeane added a comment. Looks like there is quite a few places in the LLVM build that need updating (IteratorTest.cpp, Wasm.h, ElfDumper.cpp) so that builds don't fail as we commit this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

erichkeane added a comment. The CFE stuff is pretty innocuous and I don't see a reason to stop this on our accord, but would like someone familiar with the LLVM stuff to review this at one point. Comment at: clang/lib/CodeGen/CGExpr.cpp:3694 +static bool

[PATCH] D147844: [clang][Sema]Print diagnostic warning about precedence when integer expression is used without parentheses in an conditional operator expression

erichkeane added a comment. In D147844#4651113 , @chaitanyav wrote: > Bootstrapping build failed due to -werror flag > (https://buildkite.com/llvm-project/libcxx-ci/builds/30031) > >| >

[PATCH] D148381: [Clang] Implement the 'counted_by' attribute

erichkeane added a comment. In D148381#4649683 , @void wrote: > Friendly Ping. Aaron is away at a conference this week, so hopefully he'll do another pass when he gets back next week. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D157331: [clang] Implement C23

erichkeane added a comment. In D157331#4648417 , @pirama wrote: > @aaron.ballman do you have additional comments or does this patch looks good > to merge? Aaron is away this week at a conference in Norway (see his Discourse announcement), so you're

[PATCH] D148381: [Clang] Implement the 'counted_by' attribute

erichkeane added a comment. I've got no further comments/concerns. Comment at: clang/include/clang/AST/Decl.h:4272-4275 +FieldDecl *FD = nullptr; +for (FieldDecl *Field : fields()) + FD = Field; +return FD; aaron.ballman wrote: > void wrote: >

[PATCH] D148381: [Clang] Implement the 'counted_by' attribute

erichkeane added inline comments. Comment at: clang/include/clang/AST/DeclBase.h:482 + /// Whether it resembles a flexible array member. + static bool isFlexibleArrayMemberLike( + ASTContext , const Decl *D, QualType Ty, Why isn't this a member function?

[PATCH] D135341: adds `__reference_constructs_from_temporary`

erichkeane added a comment. All of @cor3ntin 's suggestions are really good ones, I'm ok with this once he is. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135341/new/ https://reviews.llvm.org/D135341

[PATCH] D135341: [clang] adds `__reference_constructs_from_temporary`

erichkeane added a comment. Is the changes to cxx_status.html still relevant/current? Also, did we ever figure out what GCC did for a builtin here? Do they have one yet? If not, have we encouraged them to implement this one? If so, DID they implement this one? Repository: rG LLVM

[PATCH] D159490: Fix warning in MSVC

erichkeane added a comment. Fixed in 6c0b9e357617 . I didn't verify ahead of time, but hopefully the bots will yell at ME this time and I'll recognize it right away. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D159490: Fix warning in MSVC

erichkeane added a comment. In D159490#4643196 , @giulianobelinassi wrote: > In D159490#4643191 , @fdeazeve > wrote: > >> I think this may have broken builds: >> >>

[PATCH] D159490: Fix warning in MSVC

erichkeane closed this revision. erichkeane added a comment. Forgot to close the phab review automatically, but: 0323938d3c7a1a48b60a7dea8ec7300e98b4a931 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D159490: Fix warning in MSVC

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. This seems good enough to me. I'll commit it for you as before. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159490/new/

[PATCH] D159126: [Clang] Add captures to the instantiation scope of lambda call operators

erichkeane added a comment. In D159126#4642023 , @erichkeane wrote: > In D159126#4642010 , @thakis wrote: > >> Looks like the test is missing a RUN line and hence makes lit fail: >>

[PATCH] D159126: [Clang] Add captures to the instantiation scope of lambda call operators

erichkeane added a comment. In D159126#4642010 , @thakis wrote: > Looks like the test is missing a RUN line and hence makes lit fail: > http://45.33.8.238/linux/117631/step_7.txt > > Please take a look and revert for now if it takes a while to fix.

[PATCH] D157526: [clang][Sema] Remove irrelevant diagnostics from constraint satisfaction failure

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. I think this is alright. Please see if you can figure out and fix what the pre-commit clang-format issue is. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157526/new/

[PATCH] D141714: Fix ast print of variables with attributes

erichkeane added inline comments. Comment at: clang/lib/AST/DeclPrinter.cpp:312 + // printing on the left side for readbility. +else if (const VarDecl *VD = dyn_cast(D); + VD && VD->getInit() && giulianobelinassi wrote: >

[PATCH] D141714: Fix ast print of variables with attributes

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG46f3ade5083b: Fix ast print of variables with attributes (authored by giulianobelinassi, committed by erichkeane). Changed prior to commit:

[PATCH] D141714: Fix ast print of variables with attributes

erichkeane added inline comments. Comment at: clang/lib/AST/DeclPrinter.cpp:312 + // printing on the left side for readbility. +else if (const VarDecl *VD = dyn_cast(D); + VD && VD->getInit() && While compiling this, I

[PATCH] D141714: Fix ast print of variables with attributes

erichkeane added a comment. In D141714#4640825 , @giulianobelinassi wrote: > In D141714#4638225 , @erichkeane > wrote: > >> Looks fine to me, please don't commit for a day or two to give >> @aaron.ballman a

[PATCH] D159126: [Clang] Add captures to the instantiation scope of lambda call operators

erichkeane added a comment. In D159126#4640484 , @cor3ntin wrote: > Fix libc++ compile > > @erichkeane I struggle to reduce further, do you think it's worth adding this > inscrutable > test case somewhere? There's definitely value to putting it in

[PATCH] D159126: [Clang] Add captures to the instantiation scope of lambda call operators

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaLambda.cpp:2263 +LocalInstantiationScope ) +: FunctionScope(SemasRef) { + if (!isLambdaCallOperator(FD)) { cor3ntin wrote: > erichkeane wrote: > > cor3ntin wrote: > > > erichkeane

[PATCH] D159126: [Clang] Add captures to the instantiation scope of lambda call operators

erichkeane accepted this revision. erichkeane added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaLambda.cpp:2263 +LocalInstantiationScope ) +: FunctionScope(SemasRef) { + if (!isLambdaCallOperator(FD)) {

[PATCH] D159126: [Clang] Add captures to the instantiation scope of lambda call operators

erichkeane added a comment. 1 suggestion, else LGTM. Comment at: clang/lib/Sema/SemaLambda.cpp:2263 +LocalInstantiationScope ) +: FunctionScope(SemasRef) { + if (!isLambdaCallOperator(FD)) { Wonder if `LambdaScopeForCallOperatorInstantiationRAII`

[PATCH] D141714: Fix ast print of variables with attributes

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Looks fine to me, please don't commit for a day or two to give @aaron.ballman a chance to make a final comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D159358: [Clang] Realize generic lambda call operators are dependent sooner

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. This is great! Probably worth the cherry pick to Clang17 as well! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159358/new/

[PATCH] D158061: [clang] Construct ExprRequirement with SubstitutionDiagnostic on SubstFailure

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Clean up the test layout a little as a part of the commit, otherwise, LGTM! Comment at: clang/test/SemaCXX/concept-crash-on-diagnostic.cpp:28 + +// expected-error@#1

[PATCH] D141714: Fix ast print of variables with attributes

erichkeane added a comment. I think we can do better naming the tablegen'ed parts, else a bunch of smaller changes. Approach seems good enough to me, though Aaron should scroll through/make a determination after you've fixed my concerns. Comment at:

[PATCH] D158963: [CodeGen] Function multi-versioning: don't set comdat for internal linkage resolvers

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Needs a release note. I don't have a problem with the patch, but don't really understand how COMDAT works/linking works, but since you're basically the person I'd tell someone else

[PATCH] D158061: [clang] Construct ExprRequirement with SubstitutionDiagnostic on SubstFailure

erichkeane added a comment. Hi, sorry for the delay in reviewing, I'm just coming back from an extended vacation. This looks alright, except for the test. Comment at: clang/test/SemaCXX/concept-crash-on-diagnostic.cpp:26 +// Consume remaining notes/errors. +//

[PATCH] D158557: [clang] Fix crash in __builtin_strncmp and other related builtin functions

erichkeane accepted this revision. erichkeane added a comment. Still LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158557/new/ https://reviews.llvm.org/D158557 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D158869: [clang] Fix timing of propagation of MSInheritanceAttr for template instantiation declarations.

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:10112 + Specialization->addAttr(PrevDecl->getAttr()); + Consumer.AssignInheritanceModel(Specialization); +} tahonermann wrote: > I am concerned that moving the call to

[PATCH] D158557: [clang] Fix crash in __builtin_strncmp and other related builtin functions

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. I see aaron mentioned the release note and is ok with it happening on commit, so I'll undo my 'needs revision'. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158557/new/

[PATCH] D140828: [C++] Implement "Deducing this" (P0847R7)

erichkeane added a comment. Mostly just nits (plus my hatred for C style casts :) ), I didn't see any issues with the approach. Comment at: clang/include/clang/AST/ASTLambda.h:45 + return isLambdaCallOperator(DC) && + !cast(DC)->getType().isNull() && +

[PATCH] D158557: [clang] Fix crash in __builtin_strncmp and other related builtin functions

erichkeane requested changes to this revision. erichkeane added a comment. This revision now requires changes to proceed. Ah, wait, still no release note! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158557/new/ https://reviews.llvm.org/D158557

[PATCH] D158557: [clang] Fix crash in __builtin_strncmp and other related builtin functions

erichkeane added a comment. I suggest doing the -511LL value from the original bug report in the tests as well, else, fine to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158557/new/ https://reviews.llvm.org/D158557 ___ cfe-commits

[PATCH] D158557: [clang] Fix crash in __builtin_strncmp and other related builtin functions

erichkeane added a comment. Still think a codegen test for this example is VERY valuable here. Comment at: clang/test/SemaCXX/constexpr-string.cpp:681 +#pragma clang diagnostic push +#pragma clang diagnostic ignored "-Wconstant-conversion" +namespace GH64876 {

[PATCH] D158733: [Clang] Fix a crash when an invalid immediate function call appears in a cast

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. 1 nit that the build bots caught, else I'm happy. Comment at: clang/lib/Sema/SemaExpr.cpp:18364 + +assert(FD && FD->isImmediateFunction && + "could not

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

erichkeane added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:1416-1417 +not significant. This allows global constants with the same contents to be +merged. This can break global pointer identity, i.e. two different globals have +the same address. +

[PATCH] D158223: [clang] Add clang::unnamed_addr attribute that marks globals' address as not significant

erichkeane added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:1416-1417 +not significant. This allows global constants with the same contents to be +merged. This can break global pointer identity, i.e. two different globals have +the same address. +

[PATCH] D158666: [Clang] Fix linker error for function multiversioning

erichkeane added a comment. In D158666#4611494 , @eandrews wrote: > In D158666#4611481 , @erichkeane > wrote: > >> I think the .ifunc spelling was an oversight on my part when I implemented >> this, I didn't

[PATCH] D158666: [Clang] Fix linker error for function multiversioning

erichkeane requested changes to this revision. erichkeane added a comment. This revision now requires changes to proceed. Accepted before I thought about the other order, so requesting changes so we can think this through... 1 thing we DO need to consider is how this works with OLDER versions

[PATCH] D158666: [Clang] Fix linker error for function multiversioning

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. I think the .ifunc spelling was an oversight on my part when I implemented this, I didn't spend enough time investigating GCC's behavior when implementing this feature. I think the

[PATCH] D158433: [Clang] Do not change the type of captured vars when checking lambda constraints

erichkeane added a comment. I agree with this approach, and think we should backport this if at all possible. The original solution is a bit problematic, and I don't believe this solution is particularly risky. Comment at: clang/lib/Sema/SemaConcept.cpp:727 + } else +

[PATCH] D158557: [clang] Fix crash in __builtin_strncmp and other related builtin functions

erichkeane added a comment. I think there is value to ensuring we diagnose the negative-to-size_t conversion here, but I also think there is value to a CodeGen test to ensure we emit the correct value. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158557/new/

[PATCH] D158616: To allow RAV to visit initializer when bitfield and initializer both given

erichkeane added a comment. This needs a test and a release note, and it failed pre-commit CI. I don't have a good idea how to test this however, there is perhaps one of the visitor unit tests that can show this off? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D158557: [clang] Fix crash in __builtin_strncmp and other related builtin functions

erichkeane added a comment. Are there any Sema tests we can add to show that we warn/diagnose/SOMETHING on these? If someone passes a negative size, we should probably at least do the warning that it was converted/truncated. Comment at: clang/lib/AST/ExprConstant.cpp:9357

[PATCH] D155858: Add a concept AST node.

erichkeane added a comment. So I did a quick run through, I think this is a fine idea and an improvement. However, @sammccall has given some very good feedback that I'd like to have him do the final approval. Comment at: clang/include/clang/AST/ASTConcept.h:213 Expr

[PATCH] D155858: Add a concept AST node.

erichkeane added a comment. Hi! I was away on vacation, so I didn't get a chance to take a look at this. I'm currently digging my way through my 'back to work' tasks, but will keep this on my list of things to review ASAP. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D158329: [X86] Support arch=x86-64{,-v2,-v3,-v4} for target_clones attribute

erichkeane added a comment. Cant help with the RT/LLVM parts, but the clang parts are pretty much right. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13278 +llvm::Value * +CodeGenFunction::EmitX86CpuSupports(std::array FeatureMask) { Value *Result = Builder.getTrue();

[PATCH] D158329: [X86] Support arch=x86-64{,-v2,-v3,-v4} for target_clones attribute

erichkeane added inline comments. Comment at: clang/lib/CodeGen/CGBuiltin.cpp:13277 +llvm::Value * +CodeGenFunction::EmitX86CpuSupports(const uint32_t FeaturesMask[4]) { Value *Result = Builder.getTrue(); Can this be a std::array instead? The C array is

[PATCH] D157762: [WIP] Implement [[msvc::no_unique_address]]

erichkeane added a comment. still on vaca, but doing a drive-by. Comment at: clang/include/clang/Basic/Attr.td:1798 +def NoUniqueAddressMSVC : InheritableAttr, TargetSpecificAttr { + let Spellings = [CXX11<"msvc", "no_unique_address", 201803>]; + let Subjects =

[PATCH] D157526: [clang][Sema] Remove irrelevant diagnostics from constraint satisfaction failure

erichkeane added a comment. Sorry for the delay in review, I'm still on vacation. However, I don't like the idea of removing the satisfaction. I have to think about it more, but optimally we'd just not add the detail until we knew what the RHS value was, though our design doesn't

[PATCH] D153892: [NFC] Initialize class member pointers to nullptr.

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Test failure is clang-format on ASTConsumers.cpp. Fix that please (whatever it wants?) otherwise, LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D105759: Implement P2361 Unevaluated string literals

erichkeane added a comment. Just 2 small nits, otherwise this all LGTM. Comment at: clang/lib/Parse/ParseDecl.cpp:430 +} +if (Expr.isInvalid()) { + SawError = true; Please put a newline between unchained 'if' statements... it makes tehse really

[PATCH] D139837: [Clang] Implements CTAD for aggregates P1816R0 and P2082R1

erichkeane added a comment. Thanks for this! I have no concerns from what I can see, but I see the others are doing a very thorough review, so I'll let them do the approvals. Comment at: clang/include/clang/AST/DeclCXX.h:1911 setRangeEnd(EndLocation); -

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

erichkeane added a comment. In D153296#4449089 , @yronglin wrote: > Can we both check `SS->getCond()->containsErrors()` ? Maybe it can avoid > bitint's effect. WDYT? @erichkeane @shafik I'm a TOUCH uncomfortable in that this only fixes the 'current'

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

erichkeane added a comment. In D153296#4446016 , @shafik wrote: > In D153296#612 , @erichkeane > wrote: > >> So I think I'm pretty confident that the only time we would call >> `EvaluateDependentExpr` is

[PATCH] D153702: [Clang] Implement P2738R1 - constexpr cast from void*

erichkeane added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:8920 + if (HasValidResult) +CCEDiag(E, diag::note_constexpr_invalid_void_star_cast) +<< SubExpr->getType() << Info.getLangOpts().CPlusPlus26 Does

[PATCH] D151696: [x86] Remove CPU_SPECIFIC* MACROs and add getCPUDispatchMangling

erichkeane added a comment. I think this is OK, I have a slight fear we're losing a bit of the 'tune' functionality, but it is not impossible that we've never really cared about that. One concern I have is that the list was used for the resolver function, but I don't see any test changes for

[PATCH] D151753: [Clang][Sema] Do not try to analyze dependent alignment during -Wcast-align

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Needs a release note, else LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151753/new/ https://reviews.llvm.org/D151753 ___

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

erichkeane added a comment. So I think I'm pretty confident that the only time we would call `EvaluateDependentExpr` is when we are in an error condition, so I'm convinced the fix 'as is' is incorrect. The check for noteSideEffect records that we HAVE a side effect, then returns if we are OK

[PATCH] D153536: [Clang] Implement P2169 A nice placeholder with no name

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:2024 if (const VarDecl *VD = dyn_cast(D)) { +if (D->isPlaceholderVar()) + return !VD->hasInit(); Why would we get here? Doesn't line 1995 pick this up? Or am I missing where

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

erichkeane added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4989 if (SS->getCond()->isValueDependent()) { if (!EvaluateDependentExpr(SS->getCond(), Info)) return ESR_Failed; shafik wrote: > As far as I can tell `Value` will

[PATCH] D153589: [NFC] Initialize pointer fields and remove a needless null check.

erichkeane added inline comments. Comment at: clang/lib/Analysis/CFG.cpp:4156 // CFG generation for unevaluated operands. - if (S && !S->isTypeDependent() && S->isPotentiallyEvaluated()) + if (!S->isTypeDependent() && S->isPotentiallyEvaluated()) return

[PATCH] D153589: [NFC] Initialize pointer fields and remove a needless null check.

erichkeane added inline comments. Comment at: clang/lib/Analysis/CFG.cpp:4156 // CFG generation for unevaluated operands. - if (S && !S->isTypeDependent() && S->isPotentiallyEvaluated()) + if (!S->isTypeDependent() && S->isPotentiallyEvaluated()) return

[PATCH] D153589: [NFC] Initialize pointer fields and remove a needless null check.

erichkeane added inline comments. Comment at: clang/lib/Analysis/CFG.cpp:4156 // CFG generation for unevaluated operands. - if (S && !S->isTypeDependent() && S->isPotentiallyEvaluated()) + if (!S->isTypeDependent() && S->isPotentiallyEvaluated()) return

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Just a rewording of the message, else LGTM. Comment at: clang/docs/ReleaseNotes.rst:523 (`#38717 _`). +- Stop

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

erichkeane added a comment. Also needs a release note. Comment at: clang/lib/AST/ExprConstant.cpp:4893 + // Stop evaluate if E is a RecoveryExpr. + if (isa(E)) +return false; I'd probably suggest `E->containsErrors()` instead, to cover cases where we're

[PATCH] D153296: [AST] Stop evaluate constant expression if the condition expression which in switch statement contains errors

erichkeane added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:4988 + if (SS->getCond()->containsErrors() || + !EvaluateDependentExpr(SS->getCond(), Info)) return ESR_Failed; It seems to me that the 'better' solution is to

[PATCH] D153146: [CLANG] Fix potential integer overflow value in getRVVTypeSize()

erichkeane added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:9567 unsigned EltSize = Context.getTypeSize(Info.ElementType); unsigned MinElts = Info.EC.getKnownMinValue(); I think I'd suggest just changing the types of these two variables

[PATCH] D152547: [clang][NFC] Drop alignment in builtin-nondeterministic-value test

erichkeane added inline comments. Comment at: clang/test/CodeGen/builtins-nondeterministic-value.c:8 // CHECK-LABEL: entry -// CHECK: [[A:%.*]] = alloca i32, align 4 -// CHECK: store i32 [[X:%.*]], ptr [[A]], align 4 +// CHECK: [[A:%.*]] = alloca i32, align +// CHECK: store i32

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

erichkeane added a comment. In D150226#4408381 , @aaron.ballman wrote: > In D150226#4404808 , @rupprecht > wrote: > >> I suppose including this warning in `ShowInSystemHeader` with your diff >> above would be

[PATCH] D152335: [Clang] Add check to Sema::AddAlignedAttr to verify active bits is not out of range

erichkeane added a comment. In D152335#4404118 , @aaron.ballman wrote: > LGTM with a release note. > > In D152335#4404114 , @erichkeane > wrote: > >> Needs a release note. Also, the changing of order is

[PATCH] D152335: [Clang] Add check to Sema::AddAlignedAttr to verify active bits is not out of range

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Needs a release note. Also, the changing of order is POSSIBLY a change in behavior (in that we're now diagnosing 'out of range' before 'power of 2'), but I don't think we care.

[PATCH] D152197: Fix static analyzer bugs with null pointer dereferences in CheckSizelessVectorOperands()

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:11148 if (LHSType->isVLSTBuiltinType() && RHSType->isVLSTBuiltinType() && + LHSBuiltinTy && RHSBuiltinTy && Context.getBuiltinVectorTypeInfo(LHSBuiltinTy).EC != I think this

[PATCH] D152335: [Clang] Add check to Sema::AddAlignedAttr to verify active bits is not out of range

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4484 + if (!TooManyActiveBits) { +AlignVal = Alignment.getZExtValue(); +// C++11 [dcl.align]p2: So looking more closely, THIS is the problem right here. I think we probably

[PATCH] D152335: [Clang] Add check to Sema::AddAlignedAttr to verify active bits is not out of range

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4480 MaximumAlignment = std::min(MaximumAlignment, uint64_t(8192)); - if (AlignVal > MaximumAlignment) { + bool TooManyActiveBits = Alignment.getActiveBits() > llvm::APInt(64,

[PATCH] D152083: [clang] Warning for uninitialized elements in fixed-size arrays

erichkeane added a comment. Needs release notes and tests. Patch needs full-context added. Comment at: clang/include/clang/Basic/DiagnosticGroups.td:732 def UninitializedConstReference : DiagGroup<"uninitialized-const-reference">; +def UninitializedArrayElements :

[PATCH] D152259: [Clang] Reject increment of bool value in unevaluated contexts after C++17

erichkeane added a comment. In D152259#4399781 , @yronglin wrote: > Thanks for your review @erichkeane ! Seems the CI failure not caused by this > patch. I agree, I don't think it is related. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D152195: [Clang] Fix access of friend function in local class

erichkeane added a comment. In D152195#4399445 , @eandrews wrote: > In D152195#4399251 , @erichkeane > wrote: > >> I think the patch looks fine, but this needs a release note. > > Just so I know - Does every bug

[PATCH] D152069: [1/11][Clang][Type] Expand BuiltinTypeBits from 8 to 16 bits

erichkeane added inline comments. Comment at: clang/include/clang/AST/Type.h:1652 /// The kind (BuiltinType::Kind) of builtin type this is. -unsigned Kind : 8; +unsigned Kind : 16; }; It looks like the largest of the Bitfields in the union uses

[PATCH] D152194: [NFC][CLANG] Fix nullptr dereference issue found by static analyzer tool

erichkeane added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:756 if (ReceiverType->isObjCIdType() || ReceiverType->isObjCClassType() || ASTCtxt.canAssignObjCInterfaces(ReceiverObjectPtrType, TrackedType)) { const

[PATCH] D152194: [NFC][CLANG] Fix nullptr dereference issue found by static analyzer tool

erichkeane added inline comments. Comment at: clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp:756 if (ReceiverType->isObjCIdType() || ReceiverType->isObjCClassType() || ASTCtxt.canAssignObjCInterfaces(ReceiverObjectPtrType, TrackedType)) { const

[PATCH] D152195: [Clang] Fix access of friend function in local class

erichkeane added a comment. I think the patch looks fine, but this needs a release note. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152195/new/ https://reviews.llvm.org/D152195 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D152197: [NFC][CLANG] Fix static analyzer bugs with null pointer dereferences in CheckSizelessVectorOperands()

erichkeane requested changes to this revision. erichkeane added a comment. This revision now requires changes to proceed. The SA tool is incorrect here. Comment at: clang/lib/Sema/SemaExpr.cpp:9 if ((OperationKind == ACK_Arithmetic) && ((LHSBuiltinTy &&

[PATCH] D152259: [Clang] Make increment bool SFINAE-friendly

erichkeane added a comment. This should have a release note. I think the bug is asking to make this a SFINAE failure (it isn't clear what you mean by "SFINAE Friendly"). Our mechanism for that is to mark the diagnostic SFINAEError. Comment at:

[PATCH] D152186: [Clang] Add release note for 877210faa447

erichkeane accepted this revision. erichkeane added a comment. 2 nits, otherwise LGTM! Thanks! Comment at: clang/docs/ReleaseNotes.rst:333 can be controlled using ``-fcaret-diagnostics-max-lines=``. +- Clang no longer emits ``-Wunused-variable`` for variables declared with

[PATCH] D152180: [Sema] Do not emit -Wunused-variable for variables declared with cleanup attribute

erichkeane added a comment. I note I forgot to require a release note, so please add one of those (and make sure it gets incorporated in any cherry-pick that happens). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152180/new/

[PATCH] D151575: [clang][diagnostics] Always show include stacks on errors

erichkeane added a comment. I don't have much of an opinion on the commit itself. It seems that suppressing that include stack does at least SOME work to make our error-novels less 'War and Peace', but I don't really get the bug report well enough to know whether we should be doing this.

[PATCH] D151121: [Clang][UBSan] Fix the crash caused by __builtin_assume_aligned with -no-opaque-pointers enabled

erichkeane added a comment. I don't know much about this section, but I DO note that the pre-commit CI failures all seem related here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151121/new/ https://reviews.llvm.org/D151121

[PATCH] D149573: [Clang][C++23] Implement core language changes from P1467R9 extended floating-point types and standard names

erichkeane added a comment. This all LGTM, but I want @tahonermann to do a pass as well, he knows more about FP than I do, so I don't want to accept without his run-through. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149573/new/ https://reviews.llvm.org/D149573

[PATCH] D127762: [Clang][AArch64] Add/implement ACLE keywords for SME.

erichkeane added a comment. I think we need a few tests to ensure that these are properly propagated on function templates (both for Sema and codegen). Also, a handful of nits (See my list). Comment at: clang/include/clang/AST/Type.h:3958 +SME_PStateZAPreservedMask = 1

[PATCH] D152140: [Clang] Limit FunctionTypeExtraBitfields::NumExceptionType to 16 bits.

erichkeane requested changes to this revision. erichkeane added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/AST/Type.cpp:3374 auto = *getTrailingObjects(); -ExtraBits.NumExceptionType = epi.ExceptionSpec.Exceptions.size(); +

  1   2   3   4   5   6   7   8   9   10   >