[PATCH] D141324: [clang] extend external_source_symbol attribute with the USR clause

erichkeane requested changes to this revision. erichkeane added a comment. This revision now requires changes to proceed. Note both the Clang maintainer, and attributes maintainer have requested changes in some way, please let that finish before committing. CHANGES SINCE LAST ACTION

[PATCH] D141324: [clang] extend external_source_symbol attribute with the USR clause

erichkeane added a comment. In D141324#4041159 , @arphaman wrote: > In D141324#4039629 , @erichkeane > wrote: > >> I'm disturbed that the string-literal diagnostic you changed never shows up >> in the tests. I

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

erichkeane added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:565 +returning a value. They can not write to memory, but may read memory that is +immutable between invocations of the function. + lebedev.ri wrote: > erichkeane wrote: > > A

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:3863 + SourceLocation EndLoc = VD->getEndLoc(); + if (const auto *VTSD = dyn_cast(VD)) { v1nh1shungry wrote: > aaron.ballman wrote: > > erichkeane wrote: > > > Hmm... it is strange to

[PATCH] D139986: [clang][TypePrinter] Teach isSubstitutedDefaultArgument about integral types

erichkeane added a comment. In D139986#4040185 , @ldionne wrote: > @dblaikie > > We added the libc++ tests to the Clang pre-commit CI after discussing with > @erichkeane since he told me breaking libc++ was a recurring pain point, and > having a way to

[PATCH] D141324: [clang] extend external_source_symbol attribute with the USR clause

erichkeane added a comment. I'm disturbed that the string-literal diagnostic you changed never shows up in the tests. I suspect this attribute needs significantly better test coverage. Comment at: clang/docs/LanguageExtensions.rst:4815 + +An optional USR string literal can

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. 2 quick nits, otherwise LFTM. Comment at: clang/include/clang/Basic/AttrDocs.td:558 + let Content = [{ +A stronger version of ``__attribute__((pure))`` attribute.

[PATCH] D136565: [clang] Instantiate alias templates with sugar

erichkeane added a comment. Note that the Clang 16 branch is coming up approximately on January 24, and this needs to be reverted or perf-regression fixed by then. @mizvekov : If you or someone else don't have a solution/revert to this by January 13th, @aaron.ballman and I will begin the

[PATCH] D131858: [clang] Track the templated entity in type substitution.

erichkeane added a comment. Note that the Clang 16 branch is coming up approximately on January 24, and this needs to be reverted or have the issue reported by @steven_wu fixed by then. @mizvekov : If you or someone else don't have a solution/revert to this by January 13th, @aaron.ballman and

[PATCH] D136566: [clang] Instantiate concepts with sugared template arguments

erichkeane added a subscriber: aaron.ballman. erichkeane added a comment. See @aaron.ballman 's comment here: https://github.com/llvm/llvm-project/issues/59271 Since the release branch is pending, if we don't have a solution or a revert by the 13th, Aaron or I will revert this patch.

[PATCH] D140554: [C++20] Determine the dependency of unevaluated lambdas more accurately

erichkeane added a comment. Done, though I think it is more than time for you to ask for commit permissions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140554/new/ https://reviews.llvm.org/D140554

[PATCH] D140554: [C++20] Determine the dependency of unevaluated lambdas more accurately

This revision was automatically updated to reflect the committed changes. Closed by commit rG051cc460ba91: [C++20] Determine the dependency of unevaluated lambdas more accurately (authored by lime, committed by erichkeane). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D140639: clang: Fix handling of __builtin_elementwise_copysign

erichkeane added a comment. I still want someone more familiar with LLVM to review this, but code wise I think we're ok (modulus 1 suggestion I made in checkFPMathBuiltinElementType). Comment at: clang/lib/Sema/SemaChecking.cpp:2674 + +if (MagnitudeTy.getCanonicalType()

[PATCH] D140639: clang: Fix handling of __builtin_elementwise_copysign

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:2674 + +if (MagnitudeTy.getCanonicalType() != SignTy.getCanonicalType()) { + return Diag(Sign.get()->getBeginLoc(), arsenm wrote: > arsenm wrote: > > erichkeane wrote: > > >

[PATCH] D140639: clang: Fix handling of __builtin_elementwise_copysign

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:2674 + +if (MagnitudeTy.getCanonicalType() != SignTy.getCanonicalType()) { + return Diag(Sign.get()->getBeginLoc(), arsenm wrote: > erichkeane wrote: > > curleys not used for

[PATCH] D140639: clang: Fix handling of __builtin_elementwise_copysign

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:2660 + +ExprResult Magnitude = UsualUnaryConversions(TheCall->getArg(0)); +ExprResult Sign = UsualUnaryConversions(TheCall->getArg(1)); aaron.ballman wrote: > erichkeane wrote:

[PATCH] D137531: [clang] Add the check of membership in decltype for the issue #58674

erichkeane added a comment. In D137531#4028986 , @lime wrote: > Is there a place to see the log of the powerpc64le self-built buildbot? > @erichkeane It was sent via email to us: https://lab.llvm.org/buildbot#builders/121/builds/26738 Repository:

[PATCH] D140639: clang: Fix handling of __builtin_elementwise_copysign

erichkeane added a comment. In D140639#4028900 , @arsenm wrote: > In D140639#4028883 , @erichkeane > wrote: > >> 1 nit, and 1 trying to see what is going on. I don't have a good feeling >> what the purpose of

[PATCH] D140639: clang: Fix handling of __builtin_elementwise_copysign

erichkeane added a comment. 1 nit, and 1 trying to see what is going on. I don't have a good feeling what the purpose of this builtin is, nor whether it matches the desire/intent of this builtin, I'm hopeful one of the other reviewers has the ability to check that. Comment

[PATCH] D137531: [clang] Add the check of membership in decltype for the issue #58674

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG85960043d594: [clang] Add the check of membership in decltype for the issue #58674# (authored by lime, committed by erichkeane). Repository: rG

[PATCH] D139705: [clang] fix zero-initialization fix-it for variable template

erichkeane added a comment. As far as teh fix itself, I think this is fine. BUT i think there is value in waiting for Aaron to see if there is a deeper issue here. Comment at: clang/lib/Sema/SemaInit.cpp:3863 + SourceLocation EndLoc = VD->getEndLoc(); + if (const auto

[PATCH] D140664: [Sema] Don't mark deleted special member functions as non-trivial

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Code changes LGTM. Dead CFoo obviously should be used or deleted. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140664/new/

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

erichkeane added inline comments. Comment at: clang/lib/AST/MicrosoftMangle.cpp:2560 IsInLambda = true; -if (MD->isInstance()) +if (MD->isImplicitObjectMemberFunction()) HasThisQuals = true; cor3ntin wrote: > erichkeane wrote: > > This bit

[PATCH] D140807: [clang][Interp] Skip calling simple destructors

erichkeane added a comment. This still would have to call the destructors of base and member types, right? And this just means that it isn't user defined AND isn't deleted for some reason, so this doesn't mean 'trivial'. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D140554: [C++20] Determine the dependency of unevaluated lambdas more accurately

erichkeane added a comment. I agree with Corentin here, I've got no idea what the RIGHT solution looks like here, but this looks to be at least an improvement. 1 nit, plus need a release note. Otherwise LGTM. Comment at:

[PATCH] D139935: [NFC] [Doc] Fix example for AnnotateTypeDocs

erichkeane added a comment. I don't particularly know what the intent here was, so I'm hoping that @mboehme will review when he gets back from whatever vacation he's taking. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139935/new/

[PATCH] D140874: [clang][Interp] Support pointer types in compound assignment operations

erichkeane added a comment. I'm pretty far behind on reviews of the interpreter, but this one I noticed while looking th rough it. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:710 + + assert(*LT == PT_Ptr); + This is UB here if LT doesn't contain a

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

erichkeane added a comment. I did a look through, I'm quite sure I'm not going to be able to accept this, but I'm hoping the little feedback I gave helped. I can't help with the review of the mangling unfortunately, we probably need to wait for the owners of the manglings to make a decision.

[PATCH] D139436: [C2x] Relaxing requirements for va_start

erichkeane accepted this revision. erichkeane added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaChecking.cpp:7230 + Val && LangOpts.C2x && *Val == 0) +return false; aaron.ballman wrote: > erichkeane

[PATCH] D139436: [C2x] Relaxing requirements for va_start

erichkeane added inline comments. Comment at: clang/test/C/C2x/n2975.c:49 + +void foo(int a...); // expected-error {{C requires a comma prior to the ellipsis in a variadic function type}} erichkeane wrote: > . Was going to complain about the error message

[PATCH] D139436: [C2x] Relaxing requirements for va_start

erichkeane added a comment. Give others an extra chance to look at this, but other than the 1 concern, LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139436/new/ https://reviews.llvm.org/D139436 ___ cfe-commits mailing list

[PATCH] D139436: [C2x] Relaxing requirements for va_start

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:7230 + Val && LangOpts.C2x && *Val == 0) +return false; I see below that we still set the return type of hte call to Void. Do we wnat to keep that line, or at least, move

[PATCH] D139436: [C2x] Relaxing requirements for va_start

erichkeane added inline comments. Comment at: clang/lib/Parse/ParseDecl.cpp:7356 +// in C before C2x. Complain and provide the fix. We do not support +// this as an extension in earlier C language modes. +if (getLangOpts().C2x) Is this

[PATCH] D139326: [clang] Add test for CWG952

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. This appears right to me! I'd like others to take a look though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139326/new/

[PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

erichkeane added inline comments. Comment at: clang/test/Frontend/sarif-reason.cpp:15 +void g() { + f1<0>(); // expected-error{{no matching function for call to 'f1'}} + f1(); // expected-error{{no matching function for call to 'f1'}} cjdb wrote: > erichkeane

[PATCH] D124351: [Clang][WIP] Implement Change scope of lambda trailing-return-type - Take 2

erichkeane added a comment. In D124351#3950805 , @cor3ntin wrote: > Rebase. This is still a bit rough, I have a few tests to fix > and some dead code to remove. > > --- > > @erichkeane I wouldn't mind picking your brain on this. > Consider: > >

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

erichkeane added a comment. I'll take a look at rewording the docs if no one else does. I should hopefully have time next week, the rest of the patch is perhaps more important at the moment. Comment at: clang/docs/ReleaseNotes.rst:841 operator. --

[PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

erichkeane added a comment. In D138939#3964439 , @erichkeane wrote: > In D138939#3964404 , @cjdb wrote: > >> In D138939#3963496 , @erichkeane >> wrote: >> >>> In

[PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

erichkeane added a comment. In D138939#3964404 , @cjdb wrote: > In D138939#3963496 , @erichkeane > wrote: > >> In D138939#3963473 , @tschuett >> wrote: >> >>> Maybe

[PATCH] D138958: [clang] Better UX for Clang’s unwind-affecting attributes

erichkeane added inline comments. Comment at: clang/docs/ReleaseNotes.rst:841 operator. -- ``clang_Cursor_getNumTemplateArguments``, ``clang_Cursor_getTemplateArgumentKind``, - ``clang_Cursor_getTemplateArgumentType``, ``clang_Cursor_getTemplateArgumentValue`` and +-

[PATCH] D136815: [clang][Interp] Unify visiting variable declarations

erichkeane added inline comments. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1127 return false; - if (!this->emitInitGlobal(*T, *I, VD)) +} + } else { aaron.ballman wrote: > tbaeder wrote: > > aaron.ballman wrote: > > > tbaeder

[PATCH] D139090: [clang] Add test for CWG360

erichkeane added inline comments. Comment at: clang/test/CXX/drs/dr3xx.cpp:908 +public: + using A::baz; // #dr360-baz-using-decl +}; aaron.ballman wrote: > Endill wrote: > > erichkeane wrote: > > > This bookmark isn't necessary, so no reason to have it. But

[PATCH] D137043: [clang] add implicit include for Linux/gnu compatibility

erichkeane added a comment. I unfortunately am equally not informed enough to help here. The code itself looks fine to me, but I'm unable to make a value judgement on the change itself. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137043/new/

[PATCH] D138822: [clang] Add test for CWG36

erichkeane accepted this revision. erichkeane added inline comments. Comment at: clang/test/CXX/drs/dr0xx.cpp:489 + +using B::i; // expected-error {{redeclaration of using declaration}} +using C::i; // expected-error {{redeclaration of using declaration}}

[PATCH] D138914: Make evaluation of nested requirement consistent with requires expr.

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Happy now, thanks! Comment at: clang/include/clang/AST/ExprConcepts.h:428 + : Requirement(RK_Nested, +Constraint &&

[PATCH] D139090: [clang] Add test for CWG360

erichkeane added inline comments. Comment at: clang/test/CXX/drs/dr3xx.cpp:908 +public: + using A::baz; // #dr360-baz-using-decl +}; This bookmark isn't necessary, so no reason to have it. But thank you for using these! Repository: rG LLVM Github Monorepo

[PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

erichkeane added a comment. In D138939#3963473 , @tschuett wrote: > Maybe `void FormatDiagnostic(SmallVectorImpl , DiagnosticMode > mode)`instead of `void FormatDiagnostic(SmallVectorImpl )`? > To make the transition easer and future proof. I like this

[PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

erichkeane added a comment. In D138939#3958185 , @cjdb wrote: >> The clang-side interface to this seems a touch clunky, and I fear it'll make >> continuing its use/generalizing its use less likely. > > Is this w.r.t. the `FormatDiagnostic` being split

[PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

erichkeane added a comment. Herald added subscribers: Michael137, JDevlieghere. A few comments. I don't mind the approach, though I find myself wondering if the "FormatDiagnostic" call should stay the same, and choose the legacy-reason only when a sarif reason doesn't exist? Or for some level

[PATCH] D138822: [clang] Add test for CWG36

erichkeane added inline comments. Comment at: clang/test/CXX/drs/dr0xx.cpp:489 + +using B::i; // expected-error {{redeclaration of using declaration}} +using C::i; // expected-error {{redeclaration of using declaration}} Endill wrote: > erichkeane wrote:

[PATCH] D138914: Make evaluation of nested requirement consistent with requires expr.

erichkeane added inline comments. Comment at: clang/include/clang/AST/ExprConcepts.h:428 + : Requirement(RK_Nested, +Constraint && Constraint->isInstantiationDependent(), +Constraint && Constraint->containsUnexpandedParameterPack(),

[PATCH] D138822: [clang] Add test for CWG36

erichkeane added inline comments. Comment at: clang/test/CXX/drs/dr0xx.cpp:489 + +using B::i; // expected-error {{redeclaration of using declaration}} +using C::i; // expected-error {{redeclaration of using declaration}} As a nit, I prefer the 'notes' to

[PATCH] D138275: [clang][Interp] Avoid leaking init maps of local primitive arrays

erichkeane added a comment. This is REALLY making me concerned again about ownership semantics in the interpreter. I don't have any real concerns with this patch-as-is, but I DO have concerns with the architecture that makes something like this necessary. CHANGES SINCE LAST ACTION

[PATCH] D138901: [clang] Propely handle tests for open DRs in make_cxx_dr_status

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG3d280b037530: [clang] Propely handle tests for open DRs in make_cxx_dr_status (authored by Endill, committed by erichkeane). Repository: rG LLVM

[PATCH] D138901: [clang] Propely handle tests for open DRs in make_cxx_dr_status

erichkeane added a comment. Thanks @cor3ntin , I'll commit this then. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138901/new/ https://reviews.llvm.org/D138901 ___ cfe-commits mailing list

[PATCH] D138914: Make evaluation of nested requirement consistent with requires expr.

erichkeane added inline comments. Comment at: clang/include/clang/AST/ExprConcepts.h:428 + : Requirement(RK_Nested, +Constraint && Constraint->isInstantiationDependent(), +Constraint && Constraint->containsUnexpandedParameterPack(),

[PATCH] D138901: [clang] Propely handle tests for open DRs in make_cxx_dr_status

erichkeane added a comment. I like the change, thank you! I don't see anything to be concerned about, but my python is awful, so I'd like at least a 2nd set of eyes to take a look. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138901/new/

[PATCH] D138895: [clang] Update CWG2635 status

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGde36be740455: [clang] Update CWG2635 status (authored by Endill, committed by erichkeane). Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D138895: [clang] Update CWG2635 status

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. woops, thanks! Committed CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138895/new/ https://reviews.llvm.org/D138895 ___

[PATCH] D138852: CWG2635: Disallow constrained structured bindings.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG07008a8df57f: CWG2635: Disallow constrained structured bindings. (authored by erichkeane). Herald added a project: clang. Changed prior to commit:

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

erichkeane added a comment. I dont know the lexer well enough to do better than this review, but the code itself seems fine. Comment at: clang/www/cxx_dr_status.html:15651 Allow more characters in an n-char sequence -Unknown +Yes Shouldn't

[PATCH] D138859: [ODRHash] Drive attribute hashing through TableGen. NFC intended.

erichkeane added a comment. The hash there isn't the problem, its that you're adding a field to Attr.td that isn't serialized in ASTWriter/ASTReader. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138859/new/ https://reviews.llvm.org/D138859

[PATCH] D138852: CWG2635: Disallow constrained structured bindings.

erichkeane updated this revision to Diff 478351. erichkeane marked 2 inline comments as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138852/new/ https://reviews.llvm.org/D138852 Files: clang/include/clang/Basic/DiagnosticSemaKinds.td clang/lib/Sema/SemaDeclCXX.cpp

[PATCH] D138859: [ODRHash] Drive attribute hashing through TableGen. NFC intended.

erichkeane added a comment. I dont have a concern on this in general, but does it cause problems with modules built? I would think this flag needs to be written in ASTWriter/picked back up. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D138852: CWG2635: Disallow constrained structured bindings.

erichkeane added a comment. I would normally not really need a review here, but I did some stuff with the fixit that I'm not sure about/not sure how to test, so I was hoping someone could take a second look CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138852/new/

[PATCH] D138852: CWG2635: Disallow constrained structured bindings.

erichkeane created this revision. erichkeane added reviewers: clang-language-wg, aaron.ballman. Herald added a project: All. erichkeane requested review of this revision. Herald added a reviewer: jdoerfert. Herald added a subscriber: sstefan1. CWG2635 prohibits adding a constraint to a structured

[PATCH] D138851: [Clang] Permit static constexpr variables in constexpr functions

erichkeane added a comment. Can you add some tests that do meaningful things with the static variable? Particularly during constexpr evaluation time? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138851/new/ https://reviews.llvm.org/D138851

[PATCH] D138835: [clang] Update DR status to Revision 110

erichkeane added a comment. Woops, I never marked this as approved apaprently :) I definitely intended to above for anyone keeping track. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138835/new/ https://reviews.llvm.org/D138835

[PATCH] D138835: [clang] Update DR status to Revision 110

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGb1a6f2aa89c3: [clang] Update DR status to Revision 110 (authored

[PATCH] D138822: [clang] Add test for CWG36

erichkeane added a comment. BTW, I suspect you're close enough to ask for commit rights, and I'd prefer to not have to commit too many things for you, so please go through this procedure: https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access Repository: rG LLVM Github Monorepo

[PATCH] D138835: [clang] Update DR status to Revision 110

erichkeane added a comment. This looks good to me, thanks! If you post your "Name " that you'd like to have this committed as, I can do so (or someone else can beat me to it!). Comment at: clang/www/make_cxx_dr_status:161 continue - if dr.issue in (1432,2565):

[PATCH] D126818: Itanium ABI: Implement mangling for constrained friends

erichkeane added a comment. In D126818#3954137 , @rjmccall wrote: > To be clear, my opinion is just that the requested change is implementable, > as I understand it. I agree that each version of what we THINK the requested change might be is

[PATCH] D126818: Itanium ABI: Implement mangling for constrained friends

erichkeane added a comment. In D126818#3943071 , @dfrib wrote: > In D126818#3941201 , @erichkeane > wrote: > >> [...] particularly since the suggested wording says the opposite of what I >> THOUGHT the

[PATCH] D138387: [Clang] Implement static operator[]

erichkeane accepted this revision. erichkeane added a comment. LGTM, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138387/new/ https://reviews.llvm.org/D138387 ___ cfe-commits mailing list

[PATCH] D137487: [clang][Interp] Start implementing builtin functions

erichkeane added inline comments. Comment at: clang/lib/AST/Interp/Interp.h:1357 +if (InterpretBuiltin(S, PC, BID)) { + NewFrame.release(); + return true; tbaeder wrote: > erichkeane wrote: > > What is going on here? Why is this not a leak? >

[PATCH] D137487: [clang][Interp] Start implementing builtin functions

erichkeane added inline comments. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:1374 const Decl *Callee = E->getCalleeDecl(); - if (const auto *FuncDecl = dyn_cast_or_null(Callee)) { + if (const auto *FuncDecl = dyn_cast_if_present(Callee)) { const Function

[PATCH] D138749: [clang] Compare constraints before diagnosing mismatched ref qualifiers (GH58962)

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Feel free to clarify the comment yourself. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138749/new/ https://reviews.llvm.org/D138749

[PATCH] D138749: [clang] Compare constraints before diagnosing mismatched ref qualifiers (GH58962)

erichkeane added a comment. I can't find any example that breaks and needs the `AreConstraintExpressionsEqual`, so feel free to do it if you'd like. Comment at: clang/lib/Sema/SemaOverload.cpp:1324 +if ((NewRC != nullptr) != (OldRC != nullptr)) + // RC are most

[PATCH] D126818: Itanium ABI: Implement mangling for constrained friends

erichkeane added a comment. In D126818#3941165 , @dfrib wrote: > In D126818#3941036 , @erichkeane > wrote: > >> In D126818#3940898 , @dfrib wrote: >> >>> In

[PATCH] D126818: Itanium ABI: Implement mangling for constrained friends

erichkeane added a comment. In D126818#3940898 , @dfrib wrote: > In D126818#3935740 , @rjmccall > wrote: > >> I'm too often slow to actually apply edits to the ABI document. There's >> been plenty of time for

[PATCH] D126818: Itanium ABI: Implement mangling for constrained friends

erichkeane added a comment. In D126818#3935740 , @rjmccall wrote: > I'm too often slow to actually apply edits to the ABI document. There's been > plenty of time for feedback on this one; go ahead and act like it's accepted. Ah! I see! I thought last

[PATCH] D138284: Fix incorrect cast in VisitSYCLUniqueStableNameExpr

erichkeane added a comment. In D138284#3936830 , @aaron.ballman wrote: > Adding @bader as SYCL code owner. The changes look reasonable to me, but > codegen is not my area of expertise. Yeah, same to me. I wrote this originally, but I think the

[PATCH] D135750: [clang][Interp] Track initialization state of local variables

erichkeane added a comment. I don't have enough knowledge about how this works to do a better review, but I have a couple of suggestions. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:749 bool IsExtended) { -

[PATCH] D126818: Itanium ABI: Implement mangling for constrained friends

erichkeane updated this revision to Diff 475836. erichkeane added a comment. Updated to work via option-3 above, I believe this is the appropriate solution based on the itanium proposal and the C++ standard. @rjmccall, what concerns do we still have about Itanium being willing to add this? Do

[PATCH] D138115: [clang] Short-circuit evaluation in ::EvaluateAsConstantExpr

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. This seems to make sense to me. I'll accept, but please give others time to take a look. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D127762: [Clang][AArch64] Add ACLE attributes for SME.

erichkeane added a comment. I also don't see anything in this with how it works with function pointer conversions, how it affects template instantiation, overload resolution/etc. If this is a type attribute, we need to specify all of that, probably in a separate document, since this is such a

[PATCH] D137712: Correctly handle Substitution failure in concept specialization.

erichkeane added inline comments. Comment at: clang/include/clang/AST/ExprConcepts.h:103 + bool hasSubstitutionFailureInArgs() const { +return ArgsHasSubstitutionFailure; usaxena95 wrote: > erichkeane wrote: > > Does this really belong here instead of as

[PATCH] D137531: [clang] Add the check of membership in decltype for the issue #58674

erichkeane added a comment. T Comment at: clang/lib/Sema/SemaExpr.cpp:2671 // Check whether this might be a C++ implicit instance member access. // C++ [class.mfct.non-static]p3: So what I was asking above: You're making a ton of changes to this

[PATCH] D137901: [Clang] `nothrow`-implying attributes should actually manifest `nothrow` attribute (PR58798)

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8794 + case ParsedAttr::AT_Pure: +handleSimpleAttribute(S, D, AL); +if (!D->hasAttr()) { First, I'm not a fan of doing this in this list ast all, we shouldn't really have

[PATCH] D127762: [Clang][AArch64] Add ACLE attributes for SME.

erichkeane added inline comments. Comment at: clang/include/clang/AST/Type.h:3926 +SME_AttributeMask = 255 // We only support maximum 8 bits because of the +// bitmask in FunctionTypeExtraBitfields + };

[PATCH] D137531: [clang] Add the check of membership in decltype for the issue #58674

erichkeane added a comment. I dont really get the logic here, I'm in need of a much better commit message/description here before I can review this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137531/new/ https://reviews.llvm.org/D137531

[PATCH] D137712: Correctly handle Substitution failure in concept specialization.

erichkeane added a comment. I think this is a move in the right direction, and generally is probably pretty close. The new bool for `ArgsHasSubstitutionFailure` isn't something I get the need for yet though. we also need release notes. Comment at:

[PATCH] D137570: [Clang][Sema] Refactor category declaration under CheckForIncompatibleAttributes. NFC

erichkeane added a comment. I haven't dealt much with the loop hint attributes, but two of my coworkers have done extensive work in our downstream (@jyu2 and @mikerice), and likely have feedback about how this affects downstream feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D137712: Correctly handle Substitution failure in concept specialization.

erichkeane added a comment. I dont have a good hold as to why this is the solution, can you better explain the issue and the solution that you made? I will take a look when I get a chance next week, as the ISO meeting is taking up my week. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D136975: [Concepts] Correctly handle failure when checking concepts recursively

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG2cee2663338e: [Concepts] Correctly handle failure when checking concepts recursively (authored by erichkeane). Herald added a project: clang.

[PATCH] D136975: [Concepts] Correctly handle failure when checking concepts recursively

erichkeane added a comment. In D136975#3899703 , @ychen wrote: > Thanks for the patch. It looks good to me. > > About > >> Note that we DO need to be careful to make sure we still check >> constraints properly that are caused by a previous constraint,

[PATCH] D136975: [Concepts] Correctly handle failure when checking concepts recursively

erichkeane updated this revision to Diff 472298. erichkeane marked an inline comment as done. erichkeane added a comment. A quick 'quality of life' improvement, otherwise same as before. Should be ready for commit? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136975/new/

[PATCH] D136975: [Concepts] Correctly handle failure when checking concepts recursively

erichkeane marked 2 inline comments as done. erichkeane added inline comments. Comment at: clang/lib/Sema/SemaConcept.cpp:150 +namespace { +struct SatisfactionStackRAII { + Sema aaron.ballman wrote: > erichkeane wrote: > > aaron.ballman wrote: > > > Er, it'd

[PATCH] D136975: [Concepts] Correctly handle failure when checking concepts recursively

erichkeane updated this revision to Diff 472286. erichkeane added a comment. Fix issues aaron came up with. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136975/new/ https://reviews.llvm.org/D136975 Files: clang/docs/ReleaseNotes.rst

[PATCH] D136975: [Concepts] Correctly handle failure when checking concepts recursively

erichkeane marked 2 inline comments as done. erichkeane added inline comments. Comment at: clang/lib/Sema/SemaConcept.cpp:150 +namespace { +struct SatisfactionStackRAII { + Sema aaron.ballman wrote: > Er, it'd be nice for this not to shadow the name of the

<    2   3   4   5   6   7   8   9   10   11   >