[PATCH] D135175: [clang] adds `__is_bounded_array` and `__is_unbounded_array` as builtins

erichkeane accepted this revision. erichkeane added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaExprCXX.cpp:24 #include "clang/AST/RecursiveASTVisitor.h" +#include "clang/AST/Type.h" #include "clang/AST/TypeLoc.h"

[PATCH] D134128: Resubmit an implemention for constrained template template parameters [P0857R0 Part B]

erichkeane accepted this revision. erichkeane added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaConcept.cpp:614 + + static auto UnifyConstraintDepth(Sema , + const NamedDecl *Old,

[PATCH] D135287: Disallow dereferencing of void* in C++.

erichkeane updated this revision to Diff 465707. erichkeane marked an inline comment as done. erichkeane added a comment. fix the release note spelling of permanent. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135287/new/ https://reviews.llvm.org/D135287 Files:

[PATCH] D135287: Disallow dereferencing of void* in C++.

erichkeane added a comment. In D135287#3837584 , @jrtc27 wrote: > What about `__typeof__(*p)`? Yes, that would also be an error in C++, as it is on all other compilers. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135287/new/

[PATCH] D135287: Disallow dereferencing of void* in C++.

erichkeane added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6938 +def ext_typecheck_indirection_through_void_pointer_cpp +: ExtWarn<"ISO C++ does not allow indirection on operand of type %0">, + // Note: This uses a different

[PATCH] D135287: Disallow dereferencing of void* in C++.

erichkeane added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:6938 +def ext_typecheck_indirection_through_void_pointer_cpp +: ExtWarn<"ISO C++ does not allow indirection on operand of type %0">, + // Note: This uses a different

[PATCH] D135287: Disallow dereferencing of void* in C++.

erichkeane created this revision. erichkeane added a reviewer: clang-language-wg. Herald added a project: All. erichkeane requested review of this revision. as Discussed: https://discourse.llvm.org/t/rfc-can-we-stop-the-extension-to-allow-dereferencing-void-in-c/65708 There is no good reason to

[PATCH] D134128: Resubmit an implemention for constrained template template parameters [P0857R0 Part B]

erichkeane added inline comments. Comment at: clang/include/clang/Sema/SemaConcept.h:51 + // QualType of constrained template template parameter is not the same as + // its tailing version, so give it a pass here. + if (LHS.getKind() == TemplateArgument::Type &&

[PATCH] D135025: [clang][Interp] Support base class constructors

erichkeane added inline comments. Comment at: clang/test/AST/Interp/records.cpp:165 + +class A : public Base { +public: tbaeder wrote: > erichkeane wrote: > > shafik wrote: > > > How about also testing `private` and `virtual` as well as multiple bases. > > I

[PATCH] D135025: [clang][Interp] Support base class constructors

erichkeane added inline comments. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.h:263 +QualType T = E->getType(); +if (const auto *PT = dyn_cast(T)) + T = PT->getPointeeType(); We are de-pointering here, why not de-referencing? If we are OK

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

erichkeane added a comment. Sorry for the long delay, the size of this makes it tough to review, and I've been trying to make sure my concepts patch didn't get reverted. This generally looks good to me, however, I really don't like the number of bits to represent 'template parameter' being

[PATCH] D128745: [c++] implements DR692, DR1395 and tentatively DR1432, about partial ordering of variadic template partial specialization or function template

erichkeane added a comment. Is there any ability here to diagnose the difference? It would probably be helpful if at any point (in both directions!) we diagnose that our behavior changes. We did something similar for the reversible operators at one point (but WG21 might have fixed it

[PATCH] D135099: [C2x] Implement support for nullptr and nullptr_t

erichkeane added inline comments. Comment at: clang/include/clang/AST/PrettyPrinter.h:68 SuppressDefaultTemplateArgs(true), Bool(LO.Bool), -Nullptr(LO.CPlusPlus11), Restrict(LO.C99), Alignof(LO.CPlusPlus11), +Nullptr(LO.CPlusPlus11),

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG939a3d22e21f: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl (authored by erichkeane). Herald added a project: clang. Changed

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:255-258 +/// \param ForConstraintInstantiation when collecting arguments, +/// ForConstraintInstantiation indicates we should continue looking when +/// encountering a lambda generic call

[PATCH] D135011: Add sin and cos llvm builtins

erichkeane added a comment. Please clarify the commit message (review description) to better explain what it is you're doing here, the purpose/etc as well as the documentation requested by @fhahn This also needs release notes. Comment at:

[PATCH] D135088: [Clang] make canonical AutoType constraints-free

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. This looks right enough to me with the 1 nit, but let Matheus take a look before committing. Comment at: clang/lib/AST/ASTContext.cpp:699

[PATCH] D133500: [clang] Correct handling of lambdas in lambda default arguments in dependent contexts.

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. I'm OK with it as-is, and the refactor to move the replicated code would be acceptable in an NFC followup. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D133499: [clang]: Add DeclContext::dumpAsDecl().

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Please see the two comments, otherwise this LGTM. Feel free to fix the below as a part of the commit process. Comment at: clang/lib/AST/ASTDumper.cpp:240 + // the

[PATCH] D134928: [Sema] Don't treat a non-null template argument as if it were null.

erichkeane added inline comments. Comment at: clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1-11.cpp:31 IP<> ip7; // expected-error{{non-type template argument of type 'int *' is not a constant expression}} +IP<(int*)1> ip8; // expected-error {{non-type template argument does

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

erichkeane updated this revision to Diff 464360. erichkeane marked 4 inline comments as done. erichkeane added a comment. Do all the things Tom requested. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134874/new/ https://reviews.llvm.org/D134874 Files:

[PATCH] D134334: [Clang] Fix crash in isCXXDeclarationSpecifier when attempting to annotate template name

erichkeane added a comment. In D134334#3827895 , @shafik wrote: > In D134334#3805590 , @erichkeane > wrote: > >> I have no real idea what is going on here, the parser isn't an area where I >> spend much time.

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

erichkeane marked 9 inline comments as done. erichkeane added inline comments. Comment at: clang/lib/Sema/SemaConcept.cpp:510 - /*Pattern*/ nullptr, - /*LookBeyondLambda*/ true); if

[PATCH] D134654: [clang] Detect header loops

erichkeane added inline comments. Comment at: clang/test/Preprocessor/warn-loop-macro-1.h:3 +#define LOOP_MACRO_1 +// expected-warning@+1 {{#include cycle}} +#include "warn-loop-macro-1.h" aaron.ballman wrote: > For example, as a user, I would look at this

[PATCH] D134885: [Clang] Fix variant crashes from GH58028, GH57370

erichkeane added a comment. In D134885#3827535 , @royjacobson wrote: > In D134885#3827479 , @shafik wrote: > >> In D134885#3826335 , @royjacobson >> wrote: >> >>>

[PATCH] D134885: [Clang] Fix variant crashes from GH58028, GH57370

erichkeane added a comment. Only thing I can think of is the 'Aggregate' calculation for this diagnostic is going wrong somewhere. See SemaDeclcXX.cpp ~6759 for where this all happens. Aggregate IS initialized correctly to 'true' in CXXRecordDecl's DefinitionData as far as I can tell. BUT I

[PATCH] D134885: [Clang] Fix variant crashes from GH58028, GH57370

erichkeane added a comment. In D134885#3826335 , @royjacobson wrote: > Apparently some of the workers crashed with the test - > https://lab.llvm.org/buildbot/#/builders/216/builds/10556, but I couldn't > reproduce this locally. @shafik any idea why

[PATCH] D134958: [clang][Interp] Support __builtin_clz calls

erichkeane added a comment. In D134958#3827038 , @tbaeder wrote: > In D134958#3827024 , @erichkeane > wrote: > >> This seems right enough to me, though you might consider CTZ as well since >> it is equally as

[PATCH] D134958: [clang][Interp] Support __builtin_clz calls

erichkeane added a comment. This seems right enough to me, though you might consider CTZ as well since it is equally as easy. A better/more useful attempt is going to be builtin_strlen. Note that with builtins they are going to be particularly difficult because you can't execute them on the

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

erichkeane updated this revision to Diff 464238. erichkeane marked 10 inline comments as done. erichkeane added a comment. Did @shafik s suggestions. And yes, most of the work is just copy/pasted with slight changes to work in the new refactor, which does make it tougher to read, sorry about

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

erichkeane updated this revision to Diff 464003. erichkeane added a comment. Looks like that problem was fixed easy enough, so back to all tests passing :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134874/new/ https://reviews.llvm.org/D134874 Files:

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

erichkeane added inline comments. Comment at: clang/test/SemaTemplate/concepts-lambda.cpp:26 + template + auto f2 = [](auto... args) +requires (sizeof...(args) > 0) Huh... this f2 example seems to ICE for some reason, I could swear it worked, but

[PATCH] D134128: Resubmit an implemention for constrained template template parameters [P0857R0 Part B]

erichkeane added a comment. In D134128#3813708 , @lime wrote: > Well, Something happened after rebasing this patch on D126907 > . `s41` below was rejected as the constrain > generated from `template ` was no longer

[PATCH] D134874: [Concepts] Fix Concepts on generic lambda in a VarTemplateSpecDecl

erichkeane created this revision. erichkeane added reviewers: tahonermann, shafik. Herald added a project: All. erichkeane requested review of this revision. As fallout of the Deferred Concept Instantiation patch (babdef27c5 ),

[PATCH] D134286: [C2x] implement typeof and typeof_unqual

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134286/new/ https://reviews.llvm.org/D134286 ___ cfe-commits mailing list

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. LGTM! Comment at: clang/lib/AST/Decl.cpp:4480 + // the tag is anonymous and we should print it differently. + if (Name.isIdentifier() &&

[PATCH] D134749: [clang][Interp] Implement Div opcode

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Seems good enoguh to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134749/new/ https://reviews.llvm.org/D134749

[PATCH] D134744: [clang][Interp] Implement rem opcode

erichkeane accepted this revision. erichkeane added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/AST/Interp/Integral.h:213 + static bool rem(Integral A, Integral B, unsigned OpBits, Integral *R) { +*R = Integral(A.V % B.V);

[PATCH] D134744: [clang][Interp] Implement rem opcode

erichkeane added inline comments. Comment at: clang/lib/AST/Interp/Integral.h:213 + static bool rem(Integral A, Integral B, unsigned OpBits, Integral *R) { +*R = Integral(A.V % B.V); Its a touch weird we return 'bool' in all these places, rather than an

[PATCH] D133683: [c++] implements tentative DR1432 for partial ordering of function template

erichkeane accepted this revision. erichkeane added a comment. Just fix @mizvekov 's nits (the fixme, release notes, and ABI test), and LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133683/new/ https://reviews.llvm.org/D133683

[PATCH] D134461: [Clang] Warn when trying to deferencing void pointers in C

erichkeane added a comment. FWIW, I think the idea of allowing dereference of `void*` in an unevaluated context seems somewhat sensible, if sad. I suspect the kernel is not the only place to do this foolishness and take advantage of the extensions we/gcc permit for the `void` type. So I'll

[PATCH] D134542: [Concepts] Recover properly from a RecoveryExpr in a concept

This revision was automatically updated to reflect the committed changes. Closed by commit rG684a78968bd3: Reapply [Concepts] Recover properly from a RecoveryExpr in a concept (authored by erichkeane). Herald added a project: clang. Changed prior to commit:

[PATCH] D134523: [clang][Interp] Fix copy constructors with record array members

erichkeane added inline comments. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:332 + // ArrayIndex might not be set if a ArrayInitIndexExpr is being evaluated + // stand-alone, e.g. via EvaluateAsInt(). + if (!ArrayIndex) For my edification: what

[PATCH] D134578: Add missing `struct` keyword to the test p2-2.cpp

erichkeane added a comment. Er... please dont' count that 'accept', let @ChuanqiXu be the approver here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134578/new/ https://reviews.llvm.org/D134578 ___

[PATCH] D53847: [C++2a] P0634r3: Down with typename!

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Thanks for looking into it. I think you're right that it is a mistake that we accepted that without the 'struct' keyword after a quick look, so I think I am OK with this. Please give

[PATCH] D134578: Add missing `struct` keyword to the test p2-2.cpp

erichkeane added subscribers: aaron.ballman, erichkeane. erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Thanks for looking into it. I think you're right that it is a mistake that we accepted that without the 'struct' keyword after

[PATCH] D134542: [Concepts] Recover properly from a RecoveryExpr in a concept

erichkeane added a comment. In D134542#3812711 , @ychen wrote: > In D134542#3812292 , @erichkeane > wrote: > >> In D134542#3812211 , @ychen wrote: >> >>> The patch looks

[PATCH] D134542: [Concepts] Recover properly from a RecoveryExpr in a concept

erichkeane updated this revision to Diff 462568. erichkeane added a comment. Changes as suggested by @ychen. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134542/new/ https://reviews.llvm.org/D134542 Files: clang/docs/ReleaseNotes.rst clang/include/clang/AST/ASTConcept.h

[PATCH] D134542: [Concepts] Recover properly from a RecoveryExpr in a concept

erichkeane marked 2 inline comments as done. erichkeane added a comment. In D134542#3812292 , @erichkeane wrote: > In D134542#3812211 , @ychen wrote: > >> The patch looks good. Two high-level questions: >> >> -

[PATCH] D134542: [Concepts] Recover properly from a RecoveryExpr in a concept

erichkeane marked 3 inline comments as done. erichkeane added a subscriber: tahonermann. erichkeane added a comment. In D134542#3812211 , @ychen wrote: > The patch looks good. Two high-level questions: > > - Does the similar thing happen for class

[PATCH] D134542: [Concepts] Recover properly from a RecoveryExpr in a concept

erichkeane created this revision. erichkeane added a reviewer: clang-language-wg. Herald added a project: All. erichkeane requested review of this revision. Discovered by reducing a different problem, we currently assert because we failed to make the constraint expressions not dependent, since a

[PATCH] D134461: [Clang] Diagnose an error when trying to deferencing void pointers in C

erichkeane added inline comments. Comment at: clang/include/clang/Parse/Parser.h:1792 + bool *NotPrimaryExpression = nullptr, + bool IsAfterAmp = false); ExprResult ParseCastExpression(CastParseKind ParseKind,

[PATCH] D53847: [C++2a] P0634r3: Down with typename!

erichkeane added inline comments. Comment at: clang/lib/Parse/ParseDecl.cpp:5592 +bool Parser::isConstructorDeclarator(bool IsUnqualified, bool DeductionGuide, + bool IsFriend) { TentativeParsingAction TPA(*this);

[PATCH] D53847: [C++2a] P0634r3: Down with typename!

erichkeane added inline comments. Comment at: clang/lib/Parse/ParseDecl.cpp:5592 +bool Parser::isConstructorDeclarator(bool IsUnqualified, bool DeductionGuide, + bool IsFriend) { TentativeParsingAction TPA(*this);

[PATCH] D53847: [C++2a] P0634r3: Down with typename!

erichkeane added a comment. I think this is on the right track, but would like to see more work done on that fixme before we commit (or at least better understand what is causing this). I'm afraid of what else can get us into that situation besides the export-decl. Comment

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

erichkeane added a comment. In D126907#3807707 , @wlei wrote: > Tested this and confirmed the issue I reported is gone, thanks! Thank you all for the quick responses! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126907/new/

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

erichkeane added a comment. In D126907#3806760 , @Mordante wrote: > In D126907#3805606 , @erichkeane > wrote: > >> Now fully runs libcxx modules configuration as well, and passes the >> minimization examples

[PATCH] D133659: [Clang] P1169R4: static operator()

erichkeane added a comment. In D133659#3806315 , @royjacobson wrote: > Does anyone feel comfortable with looking at the CodeGen tests? They're > checking that we aren't passing a `this` argument. > > @erichkeane @shafik @lichray The codegen tests look

[PATCH] D134128: Resubmit an implemention for constrained template template parameters [P0857R0 Part B]

erichkeane added a comment. In D134128#3805675 , @lime wrote: > I have not looked deep into D126907 , but > the rule it referred seems related to something as follows: > > template concept C = true; > > template

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

erichkeane added a comment. Failure I noticed was NOT a regression: https://godbolt.org/z/MnvqP88vv CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126907/new/ https://reviews.llvm.org/D126907 ___ cfe-commits mailing list

[PATCH] D134128: Resubmit an implemention for constrained template template parameters [P0857R0 Part B]

erichkeane added a comment. In D134128#3805675 , @lime wrote: > I have not looked deep into D126907 , but > the rule it referred seems related to something as follows: You shouldn't have to look 'deep' into it, just

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

erichkeane added a comment. In D126907#3805640 , @erichkeane wrote: > I spoke too soon, I found another crash that came out of @wlei s repro from > last time. Actually, what I found was a reduction that had gone haywire before and generated invalid

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

erichkeane added a comment. I spoke too soon, I found another crash that came out of @wlei s repro from last time. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126907/new/ https://reviews.llvm.org/D126907 ___ cfe-commits mailing list

[PATCH] D134286: [C2x] implement typeof and typeof_unqual

erichkeane added inline comments. Comment at: clang/include/clang/AST/Type.h:1902 AutoTypeBitfields AutoTypeBits; +TypeOfBitfields TypeOfBits; BuiltinTypeBitfields BuiltinTypeBits; aaron.ballman wrote: > erichkeane wrote: > > So the downside to

[PATCH] D126907: Deferred Concept Instantiation Implementation Take 2

erichkeane reopened this revision. erichkeane added a comment. This revision is now accepted and ready to land. I have a replacement patch that should fix all of the bugs I'm aware of. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D126907/new/ https://reviews.llvm.org/D126907

[PATCH] D134334: [Clang] Fix crash in isCXXDeclarationSpecifier when attempting to annotate template name

erichkeane added a comment. I have no real idea what is going on here, the parser isn't an area where I spend much time. Can you ELI5? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134334/new/ https://reviews.llvm.org/D134334 ___

[PATCH] D134286: [C2x] implement typeof and typeof_unqual

erichkeane added inline comments. Comment at: clang/include/clang/AST/Type.h:1902 AutoTypeBitfields AutoTypeBits; +TypeOfBitfields TypeOfBits; BuiltinTypeBitfields BuiltinTypeBits; So the downside to doing the bitfields is that EVERY 'Type' pays

[PATCH] D134286: [C2x] implement typeof and typeof_unqual

erichkeane added a comment. I'm on the fence about re-using the same nodes for `typeof` and `typeof_unqual`. However, I have a preference to stop shuffling a bool around everywhere as parameters, and would like an enum (even if we store it as a bitfield : 1). Something like: enum class

[PATCH] D134128: Resubmit an implemention for constrained template template parameters [P0857R0 Part B]

erichkeane added a comment. One test that i need to have (that might actually end up conflicting with the above mentioned), is a reproducer that has a `ClassTemplateDecl` that is its own friend. So something like: template class C requires ...> struct S { template class C requires

[PATCH] D133856: [clang][Interp] Pass initializer through when creating variables

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Seems fine to me Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133856/new/ https://reviews.llvm.org/D133856

[PATCH] D133468: [clang] Implement divergence for TypedefType and UsingType

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. In D133468#3799609 , @mizvekov wrote: > In D133468#3799572 , @erichkeane > wrote: > >> I agree with

[PATCH] D133468: [clang] Implement divergence for TypedefType and UsingType

erichkeane added a comment. In D133468#3799557 , @mizvekov wrote: > In D133468#3799522 , @erichkeane > wrote: > >> as far as `Divergent`, I wonder if we should call it something more >> descriptive, since it

[PATCH] D132816: [clang] AST: SubstTemplateTypeParmType support for non-canonical underlying type

erichkeane added a comment. In D132816#3799535 , @mizvekov wrote: > In D132816#3799484 , @erichkeane > wrote: > >> Patch generally seems OK to me. I would vastly prefer a better commit >> message explaining the

[PATCH] D133468: [clang] Implement divergence for TypedefType and UsingType

erichkeane added a comment. Thanks for the explanation, I think that makes more sense to me. A summarized version in the commit message would be appreciated. As far as the 'asserts', i see them now in the `ASTContext` functions. as far as `Divergent`, I wonder if we should call it something

[PATCH] D134145: [Clang] Implement fix for DR2628

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. This seems right to me, thanks! I'm currently pushing to try to make our diagnostics 'check' lines more readable... it doesn't gain a ton in this case, but more to try to push for

[PATCH] D110641: Implement P0857R0 -Part B: requires clause for template-template params

erichkeane abandoned this revision. erichkeane added a comment. Taken over by someone else here: https://reviews.llvm.org/D134128 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110641/new/ https://reviews.llvm.org/D110641 ___ cfe-commits

[PATCH] D134128: Resubmit an implemention for constrained template template parameters [P0857R0 Part B]

erichkeane added a comment. Thanks for taking this over! FIRST, please make sure you re-submit this with the entire 'context' (see context-not-available), by making sure you use -U99 on your patch before uploading it. It DOES appear from the tests that we're already checking everything

[PATCH] D132816: [clang] AST: SubstTemplateTypeParmType support for non-canonical underlying type

erichkeane added a comment. Patch generally seems OK to me. I would vastly prefer a better commit message explaining the intent here. Also, not quite sure I see the need for the extra bit? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D133468: [clang] Implement divergence for TypedefType and UsingType

erichkeane added a comment. I'm still having trouble from the description and test figuring out what this means. Can you better clarify the intent/effect of this? Is the point that the type-aliases could also store a non-canonicalized type? if so, perhaps the patch should better reflect that

[PATCH] D133934: [clang][Interp] Handle sizeof() expressions

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. I think we're OK for now, LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133934/new/ https://reviews.llvm.org/D133934 ___

[PATCH] D134067: [HLSL] Enable availability attribute

erichkeane added a comment. Still looks good to me, no nits. As far as using the bookmarks, I tend to use them for things that are 'far away' instead of 'right here', and stick to the @+/@- bits for 'local' ones. Comment at: clang/test/SemaHLSL/AvailabilityMarkup.hlsl:13

[PATCH] D134067: [HLSL] Enable availability attribute

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Besides the request for re-organizing the the test files, LGTM. Comment at: clang/test/SemaHLSL/AvailabilityMarkup.hlsl:15 +void fn() { +// expected-warning@+2

[PATCH] D134067: [HLSL] Enable availability attribute

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaAvailability.cpp:198 +return Triple.getVendor() == llvm::Triple::Apple || + Triple.getOS() == llvm::Triple::ShaderModel; } This should jsut be a 'case' label added above, we are

[PATCH] D133934: [clang][Interp] Handle sizeof() expressions

erichkeane added inline comments. Comment at: clang/lib/AST/Interp/Boolean.h:50 explicit operator unsigned() const { return V; } + explicit operator int8_t() const { return V; } tbaeder wrote: > aaron.ballman wrote: > > At some point, do we want to rename

[PATCH] D134057: [clang][Interp] Start implementing record types

erichkeane added inline comments. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:316 + // Base above gives us a pointer on the stack. + const auto *FD = dyn_cast(Member); + assert(FD); I THINK Member is a ValueDecl because it could be a member function,

[PATCH] D134054: [clang][Interp] Properly destruct allocated Records

erichkeane accepted this revision. erichkeane added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/AST/Interp/Program.h:48 +// here manually so they are properly freeing their resources. +for (auto It : Records) +

[PATCH] D134054: [clang][Interp] Properly destruct allocated Records

erichkeane added a comment. Other than the pedantry above, LGTM. Comment at: clang/lib/AST/Interp/Program.h:48 +// here manually so they are properly freeing their resources. +for (auto It : Records) + It.second->~Record(); I SUSPECT that by a

[PATCH] D133853: [AST] Add msvc-specific C++11 attributes

erichkeane added a comment. In D133853#3795579 , @aaron.ballman wrote: > In D133853#3793284 , @RIscRIpt > wrote: > >> In D133853#3792518 , >> @aaron.ballman wrote: >>

[PATCH] D133934: [clang][Interp] Handle sizeof() expressions

erichkeane added inline comments. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:290 + +if (!ArgType->isConstantSizeType() || +ArgType->isDependentType()) tbaeder wrote: > erichkeane wrote: > > Documentation for `isConstantSizedType` says it

[PATCH] D133934: [clang][Interp] Handle sizeof() expressions

erichkeane added inline comments. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:290 + +if (!ArgType->isConstantSizeType() || +ArgType->isDependentType()) Documentation for `isConstantSizedType` says it isn't legal to call it on dependent or

[PATCH] D133941: [clang][Interp] Record item types in InterpStack

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. is_same_v would be an improvement, but otherwise I think this is fine. Comment at: clang/lib/AST/Interp/InterpStack.h:140 +else if constexpr (std::is_same::value

[PATCH] D134020: [clang][Interp] Handle enums

erichkeane added a comment. Also note, the CI test is from your test! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134020/new/ https://reviews.llvm.org/D134020 ___ cfe-commits mailing list

[PATCH] D134020: [clang][Interp] Handle enums

erichkeane added a comment. I can't come up with a way to get a reference to one of these, so the logic in the rest of the DeclRefExpr handling is a little MORE awkward looking now. I think I'm OK with this, but I want to give a few other folks a chance to look. Repository: rG LLVM Github

[PATCH] D133634: [clang] Allow vector of BitInt

erichkeane added a subscriber: eli.friedman. erichkeane added a comment. The CFE Changes look correct, but @eli.friedman/@craig.topper should comment as to whether this is acceptable. It still needs a few codegen tests however, to make sure the basic operations work, and that these are passed

[PATCH] D133934: [clang][Interp] Handle sizeof() expressions

erichkeane requested changes to this revision. erichkeane added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:290 +return this->emitConst( +E,

[PATCH] D133934: [clang][Interp] Handle sizeof() expressions

erichkeane added inline comments. Comment at: clang/lib/AST/Interp/ByteCodeExprGen.cpp:289 +QualType ArgType = E->getTypeOfArgument(); +return this->emitConst(E, Ctx.getASTContext().getTypeSize(ArgType)); + } You probably want `getTypeSizeInChars`.

[PATCH] D133634: [clang] Allow vector of BitInt

erichkeane added a comment. There was no motivation for disabling this other than not wanting/caring enough to define the ABI and writing sufficient tests. While I don't think there is sufficient testing here around ABI/etc, I have no problem with this. Repository: rG LLVM Github Monorepo

[PATCH] D133934: [clang][Interp] Handle sizeof() expressions

erichkeane added inline comments. Comment at: clang/lib/AST/Interp/Context.cpp:134 +llvm::Optional Context::sizeofType(QualType T) const { + if (llvm::Optional ArgT = classify(T)) +return primSize(*ArgT); This function seems pretty large for something you

[PATCH] D133941: [clang][Interp] Record item types in InterpStack

erichkeane added inline comments. Comment at: clang/lib/AST/Interp/InterpStack.h:161 + return PT_Uint64; +assert(false); + } llvm_unreachable? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133941/new/

[PATCH] D133941: [clang][Interp] Record item types in InterpStack

erichkeane added a comment. Can you clarify what the intent of this patch is? Perhaps I'm just being slow today, but I don't really get the intent here. Comment at: clang/lib/AST/Interp/InterpStack.h:135 + return PT_Ptr; +else if (std::is_same::value ||

<    4   5   6   7   8   9   10   11   12   13   >