[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size

erichkeane added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8422 +def err_int_to_float_bit_int_max_size : Error< + "cannot convert '_BitInt' operands of more than %0 bits to floating point">; Can you explain the issue here?

[PATCH] D119063: [SemaCXX] Properly scope ArgumentPackSubstitutionIndex when expanding base types

erichkeane added a comment. I think this patch makes sense to me. I Do like the ideas of making sure these instantiate properly though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119063/new/ https://reviews.llvm.org/D119063 __

[PATCH] D122237: [clang][lex] Fix failures with Microsoft header search rules

erichkeane added a comment. I think this ends up being the same as what we came up with internally, so I think this LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122237/new/ https://reviews.llvm.org/D122237 _

[PATCH] D111400: [Clang][C++2b] P2242R3: Non-literal variables [...] in constexpr

erichkeane accepted this revision. erichkeane added a comment. In D111400#3399876 , @cor3ntin wrote: > - Add tests for _Thread_local, thread > > (these should be supported in c++23 constexpr function) > > - Change /declaration/definition/ in a diagnostic

[PATCH] D122234: [clang] Link libbitint for large division of _BitInt; increase max _BitInt size

erichkeane added inline comments. Comment at: clang/lib/Lex/LiteralSupport.cpp:1232 + if (llvm::isPowerOf2_32(radix)) { +unsigned BitsPerDigit = llvm::Log2(radix); mgehre-amd wrote: > erichkeane wrote: > > Can you explain what is going on here? This isn't

[PATCH] D122083: [Concepts] Fix placeholder constraints when references are involved

erichkeane added a comment. Can you add some tests for the OTHER forms of 'auto' as well? We have `decltype(auto)` and `auto_type`, and I want to make sure whatever we do with those 'looks right'. Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:4773 + QualType MaybeAut

[PATCH] D115248: [Clang] Fix PR28101

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:3430 << D.getName().TemplateId->LAngleLoc; + if (cast(CurContext)->getDeclName() == Name) +Diag(Loc, diag::err_member_name_of_class) << Name; I see we are diagnos

[PATCH] D122083: [Concepts] Fix placeholder constraints when references are involved

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. In D122083#3402440 , @royjacobson wrote: > In D122083#3402289 , @erichkeane > wrote: > >> Can you a

[PATCH] D122083: [Concepts] Fix placeholder constraints when references are involved

erichkeane added a comment. In D122083#3402523 , @royjacobson wrote: > In D122083#3402445 , @erichkeane > wrote: > >> 1 more test I'd like to see that doesn't seem covered (ref to ptr), > > Added, good idea. > >

[PATCH] D122083: [Concepts] Fix placeholder constraints when references are involved

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

[PATCH] D115248: [Clang] Fix PR28101

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:3430 << D.getName().TemplateId->LAngleLoc; + if (cast(CurContext)->getDeclName() == Name) +Diag(Loc, diag::err_member_name_of_class) << Name; rZhBoYao wrote: > er

[PATCH] D115248: [Clang] Fix PR28101

erichkeane added a comment. Please add a 'release notes'. Otherwise, LGTM as long as the bots are ok with it! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115248/new/ https://reviews.llvm.org/D115248 ___ cfe-commits mailing list cfe-commit

[PATCH] D115248: [Clang] Fix PR28101

erichkeane added a comment. Nope, see release notes here: https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115248/new/ https://reviews.llvm.org/D115248 ___ cfe-commits m

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

erichkeane added a comment. Up for review: https://reviews.llvm.org/D123241 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122983/new/ https://reviews.llvm.org/D122983 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

erichkeane added a comment. > I think people instantiate to deeper template depths than they typically have > for template parameters, so having a deeper depth made sense to me. Hmm... the cases I can think of make me think this isn't necessarily true... It is very common to template-recurse on

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane marked 8 inline comments as done. erichkeane added inline comments. Comment at: clang/lib/Sema/SemaTemplate.cpp:4705 CheckConstraintSatisfaction( - NamedConcept, {NamedConcept->getConstraintExpr()}, Converted, + NamedConcept, {NamedConcept->get

[PATCH] D123298: [NFC] [AST] Reduce the size of TemplateParmPosition

erichkeane added a comment. I note that 'recursive' depth is limited to 1024: https://godbolt.org/z/h9bEfsToT Though I think there are other ways to get 'deeper' than that. So perhaps in any case we're just arranging deck chairs on a -post-error titanic :) Repository: rG LLVM Github Monore

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 421186. erichkeane marked 2 inline comments as done. erichkeane added a comment. Make suggestions from @ChuanqiXu CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119544/new/ https://reviews.llvm.org/D119544 Files: clang/docs/ReleaseNotes.rst c

[PATCH] D122981: [Clang] Diagnose incomplete return/param types only when function is not deleted

erichkeane added inline comments. Comment at: clang/lib/Parse/Parser.cpp:1306 + bool Delete = + Tok.is(tok::equal) && NextToken().is(tok::kw_delete) ? true : false; Decl *Res = Actions.ActOnStartOfFunctionDef(getCurScope(), D, rZhBoYao wrote: > erichkean

[PATCH] D122981: [Clang] Diagnose incomplete return/param types only when function is not deleted

erichkeane added a comment. This definitely looks like it is 'nicer' than before, a few smaller/nit-ish comments. Additionally, Phab made a REAL mess of the diff, if you could give a quick summary of what changes were actually made (vs things that were moved slightly and causing massive red/g

[PATCH] D122981: [Clang] CWG 1394: Incomplete types as parameters of deleted functions

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. I'm going to accept, I am ok with it as-is, though I would like to see @ChuanqiXu be accepting of whatever we do in Sema's enum as well. So please wait for his feedback too. ==

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 421564. erichkeane marked an inline comment as done. erichkeane added a comment. rebase for CI + comment change requested. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119544/new/ https://reviews.llvm.org/D119544 Files: clang/docs/ReleaseNote

[PATCH] D122119: [C++20][Modules] Adjust handling of exports of namespaces and using-decls.

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaModule.cpp:814-815 +diagExportedUnnamedDecl(S, UnnamedDeclKind::Namespace, D, BlockStart); + else +; // We allow an empty named namespace decl. +} else if (DC->getRedeclContext()->isFileCont

[PATCH] D123456: [C89/C2x] Diagnose calls to a function without a prototype but passes arguments

erichkeane added inline comments. Comment at: clang/docs/ReleaseNotes.rst:126 + a prototype will change behavior in C2x. Additionally, it will diagnose calls + to a function without a prototype but arguments are provided, only so long as + the ``-Wdeprecated-non-prototype`` op

[PATCH] D123182: [Concepts] Fix overload resolution bug with constrained candidates

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:2967 + "parameters!"); + for (FunctionProtoType::param_type_iterator + O = OldType->param_type_begin(), Thanks for the clarification on 'Reversed'. The comment makes

[PATCH] D123627: Correctly diagnose prototype redeclaration errors in C

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. This LGTM, please give a day or so for others to take a look. Comment at: clang/test/Sema/predefined-function.c:27 -int foobar(int); // note {{previous declaration

[PATCH] D123649: Allow flexible array initialization in C++.

erichkeane added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:346 +else if (!D.getFlexibleArrayInitChars(getContext()).isZero()) + CGM.ErrorUnsupported(D.getInit(), "flexible array init"); else if (HaveInsertPoint()) { Can you write a t

[PATCH] D122748: [Sema] Don't check bounds for function pointer

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. >> Friendly ping Unfriendly LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122748/new/ https://reviews.llvm.org/D122748 _

[PATCH] D123680: Add support for ignored bitfield conditional codegen.

erichkeane created this revision. erichkeane added reviewers: eli.friedman, aaron.ballman, tahonermann. Herald added a project: All. erichkeane requested review of this revision. Currently we emit an error in just about every case of conditionals with a 'non simple' branch if treated as an LValue.

[PATCH] D123680: Add support for ignored bitfield conditional codegen.

erichkeane added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:200 + E->IgnoreParenNoopCasts(getContext( { +if (CondOp->getObjectKind() == OK_BitField) + return EmitIgnoredConditionalOperator(CondOp); efriedma wrote: > Is there s

[PATCH] D123680: Add support for ignored bitfield conditional codegen.

erichkeane updated this revision to Diff 422539. erichkeane added a comment. Added support for chained-conditional operators as requested. All of the variants I could come up with that I was afraid would be breaking ended up having an rvalue conversion (which makes it 'work'). CHANGES SINCE L

[PATCH] D123680: Add support for ignored bitfield conditional codegen.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG6f20744b7ff8: Add support for ignored bitfield conditional codegen. (authored by erichkeane). Herald added a project: clang. Repository: rG LLVM G

[PATCH] D122983: [C11/C2x] Change the behavior of the implicit function declaration warning

erichkeane added a comment. FWIW, Aaron and I discussed this at length while he was preparing this patch. I think the current behavior (warn in C99, warn-as-error in C11/C17, hard error in C20) is about as aggressive as I'd want us to be with erroring without causing extensive heartache on our

[PATCH] D123826: Fix size of flexible array initializers, and re-enable assertions.

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Couple nits, otherwise LGTM. Comment at: clang/lib/AST/Decl.cpp:2731 +return false; + auto *List = dyn_cast(getInit()->IgnoreParens()); + if (!List) ---

[PATCH] D123840: [Clang][Sema] Fix invalid redefinition error in if/switch/for statement

erichkeane added a comment. Ah, right, we SHOULD have a release note, please add one and I'll help review the wording. Commit message is fine for a commit message, particularly with a good release note. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.or

[PATCH] D123840: [Clang][Sema] Fix invalid redefinition error in if/switch/for statement

erichkeane added a comment. >> I will add these helpers as a NFC patch after this one, that won't need a >> release note or review right? I found that awkward as well, and almost requested that you fix this, but decided against it. I'm ok with this being an NFC patch. It would be nice to grep

[PATCH] D123840: [Clang][Sema] Fix invalid redefinition error in if/switch/for statement

erichkeane added inline comments. Comment at: clang/docs/ReleaseNotes.rst:123 +- Clang should no longer incorrectly diagnose a variable declaration inside of + a lambda expression inside the scope of a if/while/for/switch init statement + as a redeclaration. So

[PATCH] D123182: [Concepts] Fix overload resolution bug with constrained candidates

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:2967 + "parameters!"); + for (FunctionProtoType::param_type_iterator + O = OldType->param_type_begin(), royjacobson wrote: > erichkeane wrote: > > Thanks for the clar

[PATCH] D123826: Fix size of flexible array initializers, and re-enable assertions.

erichkeane added inline comments. Comment at: clang/lib/AST/Decl.cpp:2735 + const Expr *FlexibleInit = List->getInit(List->getNumInits() - 1); + auto InitTy = Ctx.getAsConstantArrayType(FlexibleInit->getType()); + if (!InitTy) efriedma wrote: > erichkeane wrot

[PATCH] D123182: [Concepts] Fix overload resolution bug with constrained candidates

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. I'm happy with this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123182/new/ https://reviews.llvm.org/D123182 ___

[PATCH] D123909: [Clang] Use of decltype(capture) in parameter-declaration-clause

erichkeane added a comment. 1 nit, otherwise no comments. Comment at: clang/lib/Sema/SemaExpr.cpp:2703 + UnqualifiedId &Id) { + InMutableAgnosticContext = true; + ExprResult Res = ActOnIdExpression(S, SS, /*TemplateKwLoc*/

[PATCH] D123182: [Concepts] Fix overload resolution bug with constrained candidates

erichkeane added a comment. Pretty sure those OMP bugs are unrelated, we've been seeing those a ton lately on just about every patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123182/new/ https://reviews.llvm.org/D123182 _

[PATCH] D122663: Mark identifier prefixes as substitutable

erichkeane added a comment. @rjmccall is the expert on the Itanium ABI. Code wise, I have no real comments, though he might. As far as: >> Question: does this need to be covered by -fclang-abi-compat= when the prior >> mangling resulted in names that even llvm-cxxfilt agreed made no sense? (I

[PATCH] D122663: Mark identifier prefixes as substitutable

erichkeane added a comment. In D122663#3457130 , @hvdijk wrote: > In D122663#3456507 , @erichkeane > wrote: > >> I believe the answer here is 'yes'. We also likely need release notes. > > Thanks for the feedback

[PATCH] D122663: Mark identifier prefixes as substitutable

erichkeane added a comment. LGTM! I would like @rjmccall to take a pass if he ends up having time in the next day or two (perhaps tack on an extra day or two because of Easter), else I'll be willing to approve later in the week. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D123955: [C2x] Disallow functions without prototypes/functions with identifier lists

erichkeane added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:11243 // We really shouldn't be making a no-proto type here. + if (ArgTypes.empty() && Variadic && !getLangOpts().StrictPrototypes) How tough would it be to change this to represent

[PATCH] D123955: [C2x] Disallow functions without prototypes/functions with identifier lists

erichkeane added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:11243 // We really shouldn't be making a no-proto type here. + if (ArgTypes.empty() && Variadic && !getLangOpts().StrictPrototypes) aaron.ballman wrote: > erichkeane wrote: > > How t

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. I went to commit this, and found that a recently lit test now fails with a crash during constraint instantiation! I'll be looking into that. The example reduces to: template constexpr bool constraint = true; template void dependent(U&& u) { []() re

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. In D119544#3458966 , @cor3ntin wrote: > In D119544#3458848 , @erichkeane > wrote: > >> I went to commit this, and found that a recently lit test now fails with a >> crash during const

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. In D119544#3459045 , @tahonermann wrote: >> This is a case where the function is a template instantiation but does NOT >> have a primary template, so I have to figure out what THAT means/what I >> should be using instead. >

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. In D119544#3459169 , @tahonermann wrote: >> I wouldn't think so either? In this case the problem is that 'u' is not in >> the re-manufactured scope, I think there is a bit of work to make sure that >> lambdas ALSO get the sc

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. FWIW, I am about 95% sure I have a hold on this, plus a bunch of other cases I came up with. I likely won't get a patch up for review today (in the next hour!) unless something miraculous happens, but hopefully I'll have something tomorrow for folks to take another

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 423713. erichkeane added a comment. Herald added a subscriber: martong. Herald added a reviewer: shafik. Fixed the issue that Corentin's test came up with, added a number of others. The problem is that we didn't properly 'collect' the parameters in cases w

[PATCH] D124012: [Clang] Fix references to captured variables in dependant context.

erichkeane added a comment. In D124012#3461781 , @cor3ntin wrote: > @erichkeane I will land that later today to unstuck people relying on main, > let me know if you still want to have a look I didn't see anything on a quick pass, so LGTM! Repository:

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane marked 10 inline comments as done. erichkeane added inline comments. Comment at: clang/include/clang/AST/Decl.h:1891 +TK_DependentFunctionTemplateSpecialization, +// A Dependent function that itself is not a function. +TK_DependentNonTemplate --

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 423901. erichkeane marked 6 inline comments as done. erichkeane added a comment. Thanks for the review @ChuanqiXu ! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119544/new/ https://reviews.llvm.org/D119544 Files: clang/docs/ReleaseNotes.rst

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added a comment. Well, crud. @cor3ntin broke me again :) I went to rebase to get pre-commit to run again, and now the problem is the lambda captures. Taking a look. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119544/new/ https://reviews.llvm.org/D119544 ___

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane planned changes to this revision. erichkeane added a comment. If anyone can help here, it would be vastly appreciated... I'm simply out of ideas on how to make this work. Comment at: clang/lib/Sema/SemaConcept.cpp:496 +llvm::Optional +Sema::SetupConstraintCheckingTe

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaConcept.cpp:496 +llvm::Optional +Sema::SetupConstraintCheckingTemplateArgumentsAndScope( +FunctionDecl *FD, llvm::Optional> TemplateArgs, erichkeane wrote: > Ugh... it looks like all of this mig

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:13002 + ExprResult NewTrailingRequiresClause = + E->getCallOperator()->getTrailingRequiresClause(); erichkeane wrote: > cor3ntin wrote: > > That doesn't look right. > > At best i

[PATCH] D124012: [Clang] Fix references to captured variables in dependant context.

erichkeane added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:13165 +// when checking satisfaction. +NewTrailingRequiresClause = getDerived().TransformExpr(TRC); + So I just noticed this: What happens when NewTrailingRequir

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:13002 + ExprResult NewTrailingRequiresClause = + E->getCallOperator()->getTrailingRequiresClause(); erichkeane wrote: > erichkeane wrote: > > cor3ntin wrote: > > > That doesn't l

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 424183. erichkeane added a comment. This revision is now accepted and ready to land. Added the tests that still fail to 'concepts.cpp', I still need to figure those out. However, I switched our 'skipping of instantiation' over to use `RebuildExprInCurren

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane planned changes to this revision. erichkeane added inline comments. Comment at: clang/test/SemaTemplate/concepts.cpp:391 + + CausesFriendConstraint CFC; + FriendFunc(CFC, 1); A bunch of the tests below this all fail. CHANGES SINCE LAST ACTION htt

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane updated this revision to Diff 424241. erichkeane added a comment. This revision is now accepted and ready to land. Found that the recursive var-decl collection was incorrect, since all the values were already in the parent scopes! So I ended up being able to fix MOST of the problems

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane planned changes to this revision. erichkeane added inline comments. Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:2329 + // constraint, so we should just create a copy of the previous one. + // TODO: ERICH: Should this be RebuildExprInCurrentInstantiation he

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4203 + if (AllArgs.size() && AllArgs[0]->isValueDependent()) { +auto *Attr = AnnotateAttr::CreateWithDelayedArgs( +S.getASTContext(), AllArgs.data(), AllArgs.size(), AL); a

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

erichkeane added a comment. 1 question, otherwise happy. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8153 + + bool AttrHasVariadicArg = AL.hasVariadicArg(); + unsigned AttrNumArgs = AL.getNumArgMembers(); This still doesn't work if the VariadicExprArgument is

[PATCH] D114439: [Annotation] Allow parameter pack expansions and initializer lists in annotate attribute

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8153 + + bool AttrHasVariadicArg = AL.hasVariadicArg(); + unsigned AttrNumArgs = AL.getNumArgMembers(); steffenlarsen wrote: > erichkeane wrote: > > This still doesn't work if the Var

[PATCH] D117994: [Clang] Implement multidimentional subscript operator

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. I spent some more time looking at this, and don't see anything suspicious, and since the others didn't have anything to say (AND it is early in the cycle), I think I'm OK accepting thi

[PATCH] D119375: [Clang][Sema] Do not evaluate value-dependent immediate invocations

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. 1 nit, but otherwise I'm happy with this. Comment at: clang/test/SemaCXX/cxx2a-consteval.cpp:616 +namespace value_dependent { + A quick comment on

[PATCH] D99134: Lambdas are not necessarily locals. This resolves DR48250.

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. From my look, this seems to fix the problem, and covers the case that Richard brought up, and I have no comments, so I think I'm happy with it. Please give Richard/Aaron ~24 hrs to ta

[PATCH] D119646: [clang] Allow consteval functions in default arguments

erichkeane added inline comments. Comment at: clang/lib/AST/Decl.cpp:2816 if (auto *E = dyn_cast_or_null(Arg)) -return E->getSubExpr(); +if (!isa(E)) + return E->getSubExpr(); So what is happening here? I would still expect us to give the proper

[PATCH] D119651: [clang] Fix evaluation context type for consteval function calls in instantiations

erichkeane added a subscriber: cor3ntin. erichkeane added a comment. I'm pretty sure @cor3ntin just messed with this quite a bit, so I'd like to hear his thoughts on this. But this generally looks right on first blush. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://r

[PATCH] D119651: [clang] Fix evaluation context type for consteval function calls in instantiations

erichkeane added a comment. In D119651#3317619 , @Izaron wrote: >> Should we maybe always treat `PotentiallyEvaluated` as `ConstantEvaluated` >> in constant evaluated contexts? could that work ? > > Indeed, within this patch we can prevent similars bugs

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

erichkeane added a comment. I'm not sure this is a 'good' enough warning (by design?) to allow in -Wall either, Wall seems to be only "this is definitely a mistake" type issues, and I don't think this issue rises to that requirement (though others are likely more qualified than I to judge). I

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:6437 + NamedDecl *D = dyn_cast_or_null(Call->getCalleeDecl()); + if (!D || !D->isInStdNamespace()) +return; Quuxplusone wrote: > erichkeane wrote: > > Do we really want this? I guess

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

erichkeane added a comment. In D110641#3319787 , @cor3ntin wrote: > In D110641#3064200 , @erichkeane > wrote: > >> Ping! Does anyone have any feedback on this? I'd really like to start >> getting us caught up

[PATCH] D119095: [clang] Fix redundant functional cast in ConstantExpr

erichkeane added a comment. I think this makes sense to me, I'd like to see it captured in an AST test though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119095/new/ https://reviews.llvm.org/D119095

[PATCH] D119651: [clang] Fix evaluation context type for consteval function calls in instantiations

erichkeane requested changes to this revision. erichkeane added a comment. This revision now requires changes to proceed. Actually, based on the build bot pre-merge checks, it looks like something during self-build has hit your assert! https://buildkite.com/llvm-project/premerge-checks/builds/79

[PATCH] D114425: [clang] Add __builtin_bswap128

erichkeane added inline comments. Comment at: clang/lib/AST/ASTContext.cpp:10987 case 'i': -if (HowLong == 3) +if (HowLong == 3) { + if (!AllowInt128 && !Context.getTargetInfo().hasInt128Type()) { Please add a test in Sema as well to validate the

[PATCH] D120066: FileCheck’s regexp stumped on Windows.

erichkeane added a comment. Commit message isn't particularly good either. Perhaps something like: > [NFC] Fix debug-info-hotpatch.cpp failure due to downstream regex issue > > In our downstream, we discovered that the that the `.*` wildcard > in debug-info-hotpatch.cpp (added https://reviews.l

[PATCH] D120066: FileCheck’s regexp stumped on Windows.

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Feel free to commit with the suggestions I added above. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D120066/new/ https://reviews.llvm.o

[PATCH] D124221: Add new builtin __builtin_reflect_struct.

erichkeane added a comment. I generally just question the usefulness of this. Despite its shortcomings, the 'dump struct' has the capability of runtime introspection into a type, whereas this seems to require that the user 'know' a significant amount about the type that they are introspecting

[PATCH] D124258: [C89/C2x] Change the behavior of implicit int diagnostics

erichkeane added a comment. Some quick comments... I didn't even bother opening the tests besides checking on functionality, so hopefully those are ok too :D Comment at: clang/docs/ReleaseNotes.rst:169 + As of C2x, support for implicit int has been removed, and the warning op

[PATCH] D124258: [C89/C2x] Change the behavior of implicit int diagnostics

erichkeane added inline comments. Comment at: clang/include/clang/Basic/LangOptions.h:534 + /// Returns true if implicit int is supported at all. + bool implicitIntEnabled() const { return !CPlusPlus && !C2x; } + cor3ntin wrote: > erichkeane wrote: > > This nam

[PATCH] D88905: [Clang] Allow "ext_vector_type" applied to Booleans

erichkeane added a comment. Herald added a subscriber: StephenFan. Note the error I found with this: https://github.com/llvm/llvm-project/issues/55038 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88905/new/ https://reviews.llvm.org/D88905 __

[PATCH] D124221: Add new builtin __builtin_reflect_struct.

erichkeane added a comment. >> I think you've misunderstood; that is not required. Though with the design >> as-is, it might make sense to restrict this to being C++-only, given that >> there's not really a way to effectively use this from C. Ah, I see the example in SemaCXX. its sad we can't

[PATCH] D124258: [C89/C2x] Change the behavior of implicit int diagnostics

erichkeane added inline comments. Comment at: clang/docs/ReleaseNotes.rst:169 + As of C2x, support for implicit int has been removed, and the warning options + will have no effect. Specifying ``-Wimplicit-int`` in C89 mode will now issue + warnings instead of being a noop. ---

[PATCH] D124258: [C89/C2x] Change the behavior of implicit int diagnostics

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. I'm happy enough. I see both sides of the C89 debate enough to 'disagree and commit' on that one. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124258/new/ https://reviews.l

[PATCH] D124221: Add new builtin __builtin_reflect_struct.

erichkeane added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2434 + + bool print(int arg1, int arg2, std::initializer_list fields) { + for (auto f : fields) { rsmith wrote: > erichkeane wrote: > > rsmith wrote: > > > erichkeane wrote:

[PATCH] D124221: Add new builtin __builtin_reflect_struct.

erichkeane added a comment. In D124221#3470550 , @aaron.ballman wrote: > In D124221#3469251 , @rsmith wrote: > >> In D124221#3468706 , >> @aaron.ballman wrote: >> >>> Th

[PATCH] D122920: [Clang][CodeGen]Fix __builtin_dump_struct missing record type field name

erichkeane added a comment. @yihanaa : I'd suggest seeing the conversation that @rsmith @aaron.ballman and I are having about this builtin here: https://reviews.llvm.org/D124221 In general it looks like the three of us think that this builtin needs an overhaul in implementation/functionality in

[PATCH] D123182: [Concepts] Fix overload resolution bug with constrained candidates

erichkeane added a subscriber: nkreeger. erichkeane added a comment. @nkreeger : Can you please explain your revert, both in the revert commit message (next time), as well as the patch so that the author/rest of us have SOME hint as to why it was reverted? Frequent reverts make it painful as a

[PATCH] D122663: Mark identifier prefixes as substitutable

erichkeane added a comment. In D122663#3470687 , @hvdijk wrote: > In D122663#3457330 , @erichkeane > wrote: > >> LGTM! I would like @rjmccall to take a pass if he ends up having time in >> the next day or two (

[PATCH] D119544: Deferred Concept Instantiation Implementation

erichkeane added inline comments. Comment at: clang/test/SemaTemplate/concepts.cpp:391 + + CausesFriendConstraint CFC; + FriendFunc(CFC, 1); ChuanqiXu wrote: > erichkeane wrote: > > erichkeane wrote: > > > A bunch of the tests below this all fail. > > See these

[PATCH] D122663: Mark identifier prefixes as substitutable

erichkeane added a comment. In D122663#3471748 , @hvdijk wrote: > In D122663#3471741 , @erichkeane > wrote: > >> Ping me EOW if @rsmith doesn't respond in the meantime. It is also not >> clear to me whether you

[PATCH] D123182: [Concepts] Fix overload resolution bug with constrained candidates

erichkeane added a comment. In D123182#3471687 , @nkreeger wrote: > In D123182#3471661 , @erichkeane > wrote: > >> @nkreeger : Can you please explain your revert, both in the revert commit >> message (next time)

[PATCH] D122663: Mark identifier prefixes as substitutable

erichkeane added a comment. > llvm-cxxfilt has no option to be compatible with earlier incorrect mangling > though, so it would not help with this particular test. But it could help > with other tests, agreed. This actually wouldn't be necessary in this case: cxxfilt is already 'right', this i

[PATCH] D122663: Mark identifier prefixes as substitutable

erichkeane added a comment. In D122663#3471960 , @hvdijk wrote: > In D122663#3471866 , @erichkeane > wrote: > >> This actually wouldn't be necessary in this case: cxxfilt is already >> 'right', this is just for

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