[PATCH] D146323: inline stmt attribute diagnosing in templates

erichkeane added inline comments. Comment at: clang/test/Sema/attr-alwaysinline.cpp:48 + if constexpr (D>0) { +// expected-warning@+6{{statement attribute 'always_inline' has higher precedence than function attribute 'noinline'}} +// expected-note@#NO_DEP{{conflicting a

[PATCH] D146323: inline stmt attribute diagnosing in templates

erichkeane updated this revision to Diff 506680. erichkeane marked 5 inline comments as done. erichkeane added a comment. Changes aaron suggested. Chose zip_longest instead of enumerate. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146323/new/ https://reviews.llvm.org/D146323 Files:

[PATCH] D146426: [Sema] Fix crash on __fp16 parameters in template instantiations

erichkeane added a comment. In D146426#4207118 , @aaron.ballman wrote: > This feels like it's heading in the wrong direction -- the AST should not > have holes in it. An invalid type should be replaced by a valid type (after > diagnosing the invalid ty

[PATCH] D146420: Document the Clang policies on claiming support for a feature

erichkeane added inline comments. Comment at: clang/docs/InternalsManual.rst:3276 +Some of these feature tests are standardized, like ``__has_cpp_attribute`` or +``__cpp_decltype``, while others are Clang extensions, like ``__has_builtin``. +The common theme among all the various

[PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

erichkeane added a subscriber: asl. erichkeane added a comment. I've got the 1 concern with the mangling that I REALLY want one of our codegen owners to chime in on, otherwise this LGTM. Comment at: clang/lib/AST/ItaniumMangle.cpp:4397 +// argument. +// As proposed in

[PATCH] D146386: [MS ABI] Fix mangling references to declarations.

erichkeane added a comment. Added the code owners here. There doesn't seem to be anything questionable here to me, but I want to make sure someone who really knows what is going on takes a look. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D14638

[PATCH] D146323: inline stmt attribute diagnosing in templates

erichkeane created this revision. erichkeane added a reviewer: clang-language-wg. Herald added a project: All. erichkeane requested review of this revision. D146089 's author discovered that our diagnostics for always/no inline would null-dereference when used in

[PATCH] D146140: [clang] Properly parse variable template requires clause in lambda

erichkeane added a comment. Still LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146140/new/ https://reviews.llvm.org/D146140 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.ll

[PATCH] D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr.

erichkeane added a comment. To highlight the fix. See the rest of our release notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146089/new/ https://reviews.llvm.org/D146089 ___ cfe-commits mailing li

[PATCH] D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr.

erichkeane added a comment. Please add a release note as requested. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146089/new/ https://reviews.llvm.org/D146089 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

erichkeane added a comment. In D146178#4200821 , @rsmith wrote: > The approach of attempting to adjust the depth here is not correct, and we > should rip this whole mechanism out and reimplement it. Consider this > closely related testcase: > > templ

[PATCH] D146140: [clang] Properly parse variable template requires clause in lambda

erichkeane added a comment. In D146140#4201481 , @cor3ntin wrote: > Do we need a release note for this? Yes, good catch! @rymiel : can you please add one? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146

[PATCH] D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr.

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Please add the test cases requested, plus a Release Note. Otherwise LGTM. Comment at: clang/test/Sema/attr-alwaysinline.cpp:36 +return x; +} + --

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

erichkeane added a comment. In D146178#4199263 , @erichkeane wrote: > After a night of sleeping on it, the use of a AdjustConstraintDepth::Diff and > AdjustConstraintDepth::Value feels like a hacky solution to SOMETHING here, > though I'm not sure what

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

erichkeane added a comment. After a night of sleeping on it, the use of a AdjustConstraintDepth::Diff and AdjustConstraintDepth::Value feels like a hacky solution to SOMETHING here, though I'm not sure what yet. The depth of a template shouldn't change between the equality and constraint checki

[PATCH] D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr.

erichkeane added inline comments. Comment at: clang/test/Sema/attr-alwaysinline.cpp:36 +return x; +} + craig.topper wrote: > erichkeane wrote: > > craig.topper wrote: > > > erichkeane wrote: > > > > erichkeane wrote: > > > > > craig.topper wrote: > > > >

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

erichkeane added inline comments. Comment at: clang/test/SemaTemplate/concepts-out-of-line-def.cpp:143 +template <> +template +constexpr int S<1>::constrained_method() { return CONSTRAINED_METHOD_1; } Would also like tests with packs both at this function level,

[PATCH] D146178: [Clang][Sema] Fix comparison of constraint expressions

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaConcept.cpp:738 +public: + enum DepthAdjustmentKind { Diff, Value }; + I don't really get what you're getting at with "Diff" and "Value"? Those names don't really seem to make sense to me?

[PATCH] D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr.

erichkeane added inline comments. Comment at: clang/test/Sema/attr-alwaysinline.cpp:36 +return x; +} + craig.topper wrote: > erichkeane wrote: > > erichkeane wrote: > > > craig.topper wrote: > > > > erichkeane wrote: > > > > > craig.topper wrote: > > > >

[PATCH] D131307: [Clang] Allow downgrading to a warning the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

erichkeane added a comment. In D131307#4197767 , @rsmith wrote: >> Based on feedback we will provide users to the ability to downgrade this >> this diagnostic to a waring to allow for a transition period. We expect to >> turn this diagnostic to an error

[PATCH] D145851: [Clang][Sema] Fix incorrect deletion of default constructors for some unions

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

[PATCH] D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr.

erichkeane added inline comments. Comment at: clang/test/Sema/attr-alwaysinline.cpp:36 +return x; +} + erichkeane wrote: > craig.topper wrote: > > erichkeane wrote: > > > craig.topper wrote: > > > > erichkeane wrote: > > > > > Can you add a test that show

[PATCH] D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr.

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaStmtAttr.cpp:236 const Decl *Decl = CallExpr->getCalleeDecl(); if (Decl->hasAttr() || Decl->hasAttr()) S.Diag(St->getBeginLoc(), diag::warn_function_stmt_attribute_precedence) Same

[PATCH] D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr.

erichkeane added inline comments. Comment at: clang/test/Sema/attr-alwaysinline.cpp:36 +return x; +} + craig.topper wrote: > erichkeane wrote: > > Can you add a test that shows that we warn on instantiation? This > > shouldn't be a dependent declrefexpr

[PATCH] D146140: [clang] Properly parse variable template requires clause in lambda

erichkeane added a comment. In D146140#4196919 , @rymiel wrote: > In D146140#4196886 , @erichkeane > wrote: > >> Ah, also, before commit, ensure that pre-commit CI passes. > > Yep, found an issue already ;; > > I

[PATCH] D146140: [clang] Properly parse variable template requires clause in lambda

erichkeane added a comment. Ah, also, before commit, ensure that pre-commit CI passes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146140/new/ https://reviews.llvm.org/D146140 ___ cfe-commits mailing l

[PATCH] D146140: [clang] Properly parse variable template requires clause in lambda

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Thank you! This looks good to me. I presume you don't have commit access, so if you give me your "SomeName ", I can commit this for you. Repository: rG LLVM Github Monorepo CHANG

[PATCH] D146140: [clang] Properly parse variable template requires clause in lambda

erichkeane added inline comments. Comment at: clang/lib/Parse/ParseExprCXX.cpp:1283 ParseScope LambdaScope(this, Scope::LambdaScope | Scope::DeclScope | Scope::FunctionDeclarationScope | Ok, last bit, I promise :) I see t

[PATCH] D146140: [clang] Properly parse variable template requires clause in lambda

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaConcept.cpp:116 // primary-expression, and complain that it is of non-bool type. - (NextToken.is(tok::l_paren) && + (NextToken.is(tok::l_paren) && !IsLambdaRequiresClause &&

[PATCH] D146140: [clang] Properly parse variable template requires clause in lambda

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaConcept.cpp:116 // primary-expression, and complain that it is of non-bool type. - (NextToken.is(tok::l_paren) && + (NextToken.is(tok::l_paren) && !IsLambdaRequiresClause &&

[PATCH] D146089: [Sema] Fix null pointer dereference handleAlwaysInlineAttr.

erichkeane added inline comments. Comment at: clang/test/Sema/attr-alwaysinline.cpp:36 +return x; +} + Can you add a test that shows that we warn on instantiation? This shouldn't be a dependent declrefexpr when instantiated. Additionally, this would ma

[PATCH] D145851: [Clang][Sema] Fix incorrect deletion of default constructors for some unions

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. In D145851#4194154 , @royjacobson wrote: > Fix the codegen test, add a standard ref to the comment. No worries, there is definitely somewhat

[PATCH] D145851: [Clang][Sema] Fix incorrect deletion of default constructors for some unions

erichkeane added inline comments. Comment at: clang/test/CodeGen/union-non-trivial-member.cpp:23 + +// CHECK: define dso_local void @_Z1fv() {{.*}} { +// CHECK-NEXT: entry: Comment at: clang/test/CodeGen/union-non-trivial-member.cpp:24 +/

[PATCH] D145892: [SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag

erichkeane added a comment. In D145892#4193522 , @HerrCai0907 wrote: > Hello Erichkeane, I have the access now, maybe I can push it by myself. > Thanks again! Alright, let me know if you need any help! -Erich CHANGES SINCE LAST ACTION https://revie

[PATCH] D145892: [SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag

erichkeane added a comment. In D145892#4193428 , @HerrCai0907 wrote: > I don't have write access now. Could you kindly help me to commit it? Sure, just give me the name and email address you'd like it committed as. CHANGES SINCE LAST ACTION https://

[PATCH] D145892: [SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag

erichkeane added a comment. Looks good! Thanks! Commit when you're ready. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145892/new/ https://reviews.llvm.org/D145892 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.

[PATCH] D145892: [SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. LGTM with 2 nits. Comment at: clang/docs/ReleaseNotes.rst:196 (`#60936 `_) +- Fix crash when parsing fold expres

[PATCH] D145892: [SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag

erichkeane added inline comments. Comment at: clang/docs/ReleaseNotes.rst:173 by prioritizing ``-Wunreachable-code-fallthrough``. +- Clang won't crash when fold expression contains a delayed typos correction. This should go in the `Bug Fixes In This Version`

[PATCH] D145892: [SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag

erichkeane added a comment. In D145892#4193085 , @HerrCai0907 wrote: > Should I add release note in `clang/docs/ReleaseNotes.rst` or any other way? Yes, that is the correct place. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145892/new/ http

[PATCH] D143479: [Clang] Emit error when caller cannot meet target feature requirement from always-inlining callee

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Thanks for your patience, I was buried trying to get the 16.0 release done. This LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D14

[PATCH] D145892: [SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag

erichkeane added a comment. In D145892#4191220 , @HerrCai0907 wrote: > I cannot find an example that throw `error: expression contains unexpanded > parameter pack 'T'` in fold expression because code like ` (foo(10 + > (static_cast(1))) + ...);` can be

[PATCH] D145851: [Clang][Sema] Fix incorrect deletion of default constructors for some unions

erichkeane added inline comments. Comment at: clang/test/CodeGen/union-non-trivial-member.cpp:30 +// CHECK-NEXT: +// CHECK-NEXT: define linkonce_odr dso_local void @_ZN2UnionIntC2Ev(ptr noundef nonnull align 4 dereferenceable(4) %this) unnamed_addr #1 comdat align 2 { +// CHEC

[PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

erichkeane added inline comments. Comment at: clang/www/cxx_status.html:1067 + Reference type template arguments referring to instantiation-dependent objects and subobjects + (i.e. delcared inside a template but neither type- nor value-dependent) aren't fully

[PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

erichkeane added subscribers: eli.friedman, gribozavr2, akyrtzi, gribozavr. erichkeane added inline comments. Comment at: clang/include/clang/AST/TemplateBase.h:88 +/// so cannot be dependent. +UncommonValue, + bolshakov-a wrote: > shafik wrote: > > erich

[PATCH] D128440: [WebAssembly] Initial support for reference type funcref in clang

erichkeane added a comment. First, we've been dealing with the 16.0 release process/regressions, which unfortunately got us pretty far behind on reviews. Sorry about that! I have only 1 concern here, otherwise, this LGTM. I'll leave it to Aaron to make the final 'value judgement' of if we wan

[PATCH] D145851: [Clang][Sema] Fix incorrect deletion of default constructors for some unions

erichkeane added a comment. In D145851#4189916 , @royjacobson wrote: > In D145851#4189158 , @erichkeane > wrote: > >> Generally looks good to me. Do we do anything special if there are multiple >> initializers

[PATCH] D145892: [SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag

erichkeane added a comment. In D145892#4189603 , @HerrCai0907 wrote: > The idea is that: > > 1. If function return an invalid ExprResult, this error should be handled in > the callee > 2. If function return valid Expr, the potential error should be hand

[PATCH] D145892: [SemaCXX]use CorrectDelayedTyposInExpr in ActOnCXXFoldExpr only when Diag

erichkeane added a comment. This needs a release note. Also, the patch message doesn't do a good job explaining what is going on here. Also, I'm not sure this is the right answer. The purpose of `CorrectDelayedTyposInExpr` is, in part, to make sure we emit the diagnostics of all child expres

[PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

erichkeane added a comment. In D140996#4189452 , @bolshakov-a wrote: > Sorry! It's my first time using Phabricator. Maybe, the problem occurs > because I've solved the issue with Arcanist just by means of copy-pasting > patches into "Update Diff" Web G

[PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

erichkeane added a comment. Patch seems to be missing all the context. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140996/new/ https://reviews.llvm.org/D140996 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/c

[PATCH] D142692: [clang] Store the template param list of an explicit variable template specialization

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. sorry for the delay, just started unburying myself from the 16.0 release process. Its unfortunate we don't have a great way to test this, but I can see the value of having it anyway i

[PATCH] D145851: [Clang][Sema] Fix incorrect deletion of default constructors for some unions

erichkeane added a comment. Generally looks good to me. Do we do anything special if there are multiple initializers? Also, can we have a codegen test that validates that we actually construct it correctly (and perhaps a constexpr test for the same!)? Comment at: clang/lib/

[PATCH] D145491: [clang] Replace Member Expressions During Instantiation If Necessary

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Thanks for the ping. I don't have any better ideas/other thoughts, so LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145491/new/ https://reviews.llvm.org/D145491 __

[PATCH] D145284: WIP [clang] adds capabilities for SARIF to be written to file

erichkeane added a subscriber: jansvoboda11. erichkeane added a comment. Patch topic needs "WIP" out of it I think? Just some WIP comments, I don't believe I'd be the one to approve this anyway. Comment at: clang/include/clang/Basic/DiagnosticOptions.h:109 + /// The file to

[PATCH] D144405: [clang][pp] Handle attributes defined by plugin in __has_attribute

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. So here's a potential idea for future development: It isn't uncommon/untypical for an attribute to want to return something other than '1', for 'version' (usually an integral value re

[PATCH] D144402: [clang] Move ParsedAttrInfo from Sema to Basic (NFC)

erichkeane added a comment. I want to wait for @aaron.ballman to answer, but i think this is generally OK. Note the motivation is the ability to check this from __has_cpp_attribute/__has_c_attribute/etc. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.o

[PATCH] D144405: [clang][pp] Handle attributes defined by plugin in __has_attribute

erichkeane added a comment. Looks like this doesn't compile pre-commit, though no idea if that is a patch-stack issue. Other than test, patch looks fine. Comment at: clang/test/Frontend/plugin-attribute-pp.cpp:1 +// RUN: split-file %s %t +// RUN: %clang -fplugin=%llvmshlibdir

[PATCH] D145769: [clang] Extract ParsedAttrInfo::hasSpelling method (NFC)

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Looks like you have a clang-format fix to do (see pre-commit CI), else, LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145769/new/

[PATCH] D145605: Revert two patches to fix GH58452 regression

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rGacecf68c8b7c: Revert two patches to fix GH58452 regression (authored by erichkeane). Herald added a project: clang. Repos

[PATCH] D145605: Revert two patches to fix GH58452 regression

erichkeane updated this revision to Diff 503756. erichkeane added a comment. clang-format fixes, going to make sure this passes CI, then commit CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145605/new/ https://reviews.llvm.org/D145605 Files: clang/include/clang/Sema/Sema.h clang/in

[PATCH] D145605: Revert two patches to fix GH58452 regression

erichkeane updated this revision to Diff 503479. erichkeane added a comment. Add tests from regression bug report. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145605/new/ https://reviews.llvm.org/D145605 Files: clang/include/clang/Sema/Sema.h clang/include/clang/Sema/SemaInternal

[PATCH] D145605: Revert two patches to fix PR58452 regression

erichkeane added a comment. Going to add 1 more set of test beyond the revert, which is the tests from that regression report. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145605/new/ https://reviews.llvm.org/D145605 ___ cfe-commits mailing

[PATCH] D145605: Revert two patches to fix PR58452 regression

erichkeane created this revision. erichkeane added reviewers: tstellar, aaron.ballman. Herald added a project: All. erichkeane requested review of this revision. PR58452 is a regression in the 16.0 release branch caused by both: b8a1b698afb2fc84819c7596090aabf4d826b436

[PATCH] D145270: Add codegen for llvm exp/exp2 elementwise builtins

erichkeane added inline comments. Comment at: clang/docs/LanguageExtensions.rst:642 T __builtin_elementwise_log10(T x) return the base 10 logarithm of x floating point types + T __builtin_elementwise_exp(T x)returns the base-

[PATCH] D145270: Add codegen for llvm exp/exp2 elementwise builtins

erichkeane added a comment. Patch itself seems fine enough, but I want to give others a chance to poke at it. The name makes me grumbly, but if there is sufficient 'prior art' here, I'm ok with it. Comment at: clang/docs/LanguageExtensions.rst:642 T __builtin_elementwise_l

[PATCH] D145491: [clang] Replace Member Expressions During Instantiation If Necessary

erichkeane added a comment. Other than the two auto's missing a *, I think this is alright. I want to think about it/give others a chance to review, so please ping this in a few days if it hasn't been accepted by then. Comment at: clang/lib/Sema/TreeTransform.h:2808 +

[PATCH] D145408: Fix false positive with unreachable C++ catch handlers

erichkeane added inline comments. Comment at: clang/docs/ReleaseNotes.rst:186 + `#61177 `_ in anticipation + of `CWG2699 _` being accepted by WG21. This link seems broken? I get 40

[PATCH] D143803: [clang][alias|ifunc]: Add a diagnostic for mangled names

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG9306ef9750b7: [clang][alias|ifunc]: Add a diagnostic for mangled names (authored by 0xdc03, committed by erichkeane). Changed prior to commit: htt

[PATCH] D143803: [clang][alias|ifunc]: Add a diagnostic for mangled names

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Not thrilled about 'mangled name' still, but I can't think of anything better after all this time, so LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

[PATCH] D144405: [clang][pp] Handle attributes defined by plugin in __has_attribute

erichkeane added inline comments. Comment at: clang/lib/Basic/Attributes.cpp:46 + for (auto &AttrPlugin : getAttributePluginInstances()) { +for (auto &S : AttrPlugin->Spellings) + if (S.Syntax == Syntax && S.NormalizedFullName == Name) wanders wrote: >

[PATCH] D144405: [clang][pp] Handle attributes defined by plugin in __has_attribute

erichkeane added a comment. This needs a release note. Otherwise I just have a preference for the 'is this spelling/syntax combo provided by this attribute' being in the plugin infrastructure. It DOES make me wonder however, what happens if TWO plugins provide a different 'true' answer for th

[PATCH] D144404: [clang] Extract function for generated part of clang::hasAttribute (NFC)

erichkeane added a comment. I see now having seen the other review, LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144404/new/ https://reviews.llvm.org/D144404 ___ cfe-commits mailing list cfe-commi

[PATCH] D144404: [clang] Extract function for generated part of clang::hasAttribute (NFC)

erichkeane added inline comments. Comment at: clang/lib/Basic/Attributes.cpp:39 -#include "clang/Basic/AttrHasAttributeImpl.inc" + int res = hasAttributeImpl(Syntax, Name, ScopeName, Target, LangOpts); + if (res) Why is this not just return `hasAttributeImpl(

[PATCH] D144626: [C++20] [Modules] Trying to compare the trailing require clause of the primary template when performing ODR checking

erichkeane added a comment. In D144626#4157240 , @ChuanqiXu wrote: > This is another revision (https://reviews.llvm.org/D144707) which shouldn't > be related with libcxx's modular build. So the failures should be irrelevant > with the revision. @erichke

[PATCH] D127259: [CodeGen] guarantee templated static variables are initialized in the reverse instantiation order

erichkeane accepted this revision. erichkeane added a comment. I don't see much compile-time concerns myself, and this seems to fix a pretty serious regression, I think this is acceptable once Aaron and the author think so as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTI

[PATCH] D144802: clang: Add __builtin_elementwise_round

erichkeane added a comment. Yep! this does need a release note. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144802/new/ https://reviews.llvm.org/D144802 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/

[PATCH] D143803: [clang][alias|ifunc]: Add a diagnostic for mangled names

erichkeane added a subscriber: shafik. erichkeane added a comment. In D143803#4155345 , @0xdc03 wrote: > In D143803#4155266 , @erichkeane > wrote: > >> I think updating that test with this additional note is the

[PATCH] D143803: [clang][alias|ifunc]: Add a diagnostic for mangled names

erichkeane added a comment. In D143803#4154736 , @0xdc03 wrote: > Okay, I have now modified the diagnostic to look something like (as per the > discussion at > https://discord.com/channels/636084430946959380/636732781086638081/1079356357024694363): > >

[PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

erichkeane added inline comments. Comment at: clang/include/clang/AST/ASTContext.h:3036 if (!std::is_trivially_destructible::value) { - auto DestroyPtr = [](void *V) { static_cast(V)->~T(); }; - AddDeallocation(DestroyPtr, Ptr); + auto DestroyPtr = [](void *V)

[PATCH] D142224: [Support] Emulate SIGPIPE handling in raw_fd_ostream write for Windows

erichkeane added inline comments. Comment at: llvm/lib/Support/Windows/Signals.inc:834 + int RetCode = (int)EP->ExceptionRecord->ExceptionCode; + if (RetCode == (0xE000 | EX_IOERR)) +return; aganea wrote: > erichkeane wrote: > > This patch seems to caus

[PATCH] D135495: [clang-tidy] handle exceptions properly in `ExceptionAnalyzer`

erichkeane added a comment. In D135495#4151238 , @isuckatcs wrote: > I'm not sure if I can run clang targetting AArch64 on an X86 machine but I > tried it regardless and I get compiler errors on startup, so I can't debug > this issue at the moment. > B

[PATCH] D142224: [Support] Emulate SIGPIPE handling in raw_fd_ostream write for Windows

erichkeane added inline comments. Comment at: llvm/lib/Support/Windows/Signals.inc:834 + int RetCode = (int)EP->ExceptionRecord->ExceptionCode; + if (RetCode == (0xE000 | EX_IOERR)) +return; This patch seems to cause a self-build Werror regression. The

[PATCH] D139028: [RFC][clang] Add attribute-like keywords

erichkeane added a comment. In D139028#4150761 , @aaron.ballman wrote: > One thing that might also help is to split this into a few stages. 1) Add the > new general functionality, 2) Replacing the existing implementation of > keyword attributes with th

[PATCH] D143803: [clang][alias|ifunc]: Add a diagnostic for mangled names

erichkeane added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:359-360 +if (ND->getName() == GV->getName()) { + Diags.Report(Location, diag::note_alias_requires_mangled_name) + << GV->getName() << Name; +} ---

[PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

erichkeane added inline comments. Comment at: clang/include/clang/AST/ASTContext.h:3036 if (!std::is_trivially_destructible::value) { - auto DestroyPtr = [](void *V) { static_cast(V)->~T(); }; - AddDeallocation(DestroyPtr, Ptr); + auto DestroyPtr = [](void *V)

[PATCH] D143803: [clang][alias|ifunc]: Add a diagnostic for mangled names

erichkeane added a comment. >> which confuses me because an extern "C" block is not supposed to mangle any >> names, right? Appreciate any inputs on this. That IS strange, that has internal linkage, but so the 'extern "C"' doesn't do anything to it, so we choose to just mangle it. I guess ther

[PATCH] D144626: [C++20] [Modules] Trying to compare the trailing require clause of the primary template when performing ODR checking

erichkeane added a comment. That looks reasonable to me, though I fear all the libcxx failures are going to be related to this, please make sure to check them out! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144626/new/ https://reviews.llvm.org/D144626 __

[PATCH] D144626: [C++20] [Modules] Provide OriginalTrailingRequiresClause for serializer to perform ODR-Checking correctly

erichkeane added a comment. Woops, I just saw my fixme :D I forgot we needed to do that for comparison of constraints. That said I dont think you need to store it, I think we can just pick up the 'original' one from the primary template. I did work at one point to make sure that the get-inst

[PATCH] D144626: [C++20] [Modules] Provide OriginalTrailingRequiresClause for serializer to perform ODR-Checking correctly

erichkeane added a comment. I think the problem is more that we are modifying the expression in the AST. I don't think now that we have the 'deferred concepts instantiation' that we should be storing/saving the updated requires clause back to the AST, right? We should be re-evaluating it (wit

[PATCH] D144572: [C++20] Stop claiming full support for consteval (for the moment!)

erichkeane added a comment. We've discussed this offline, and I think this is the right thing to do, but I'll let others comment before approving. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144572/new/ https://reviews.llvm.org/D144572

[PATCH] D144334: [Clang] Add C++2b attribute [[assume(expression)]]

erichkeane added inline comments. Comment at: clang/lib/CodeGen/CGStmt.cpp:723-727 +case attr::Assume: { + llvm::Value *ArgValue = EmitScalarExpr(cast(A)->getCond()); + llvm::Function *FnAssume = CGM.getIntrinsic(llvm::Intrinsic::assume); + Builder.CreateCall(F

[PATCH] D144334: [Clang] Add C++2b attribute [[assume(expression)]]

erichkeane added a comment. In D144334#4142462 , @Izaron wrote: > In D144334#4141646 , @erichkeane > wrote: > >> I'm on the fence as to whether we want to implement this feature at all. As >> was discussed exte

[PATCH] D143670: Stop claiming we support [[carries_dependency]]

erichkeane added a comment. In D143670#4142351 , @aaron.ballman wrote: > In D143670#4133677 , @rsmith wrote: > >> Until or unless a C++ DR permits us to define >> `__has_cpp_attribute(carries_dependency)` to any

[PATCH] D143840: [clang] Add the check of membership for the issue #58674 and improve the lookup process

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. I don't have a great idea of this section of code, but as code-owner I guess I'm stuck approving/disapproving of this :) Barring additional information, I think I have to accept this

[PATCH] D144334: [Clang] Add C++2b attribute [[assume(expression)]]

erichkeane added a comment. So one thing to note here: I'm on the fence as to whether we want to implement this feature at all. As was discussed extensively during the EWG meetings on this: multiple implementers are against this attribute for a variety of reasons, and at least 1 other implemen

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16806 +bool InTemplateDefinition = +getLangOpts().CPlusPlus && CurContext->isDependentContext(); + shafik wrote: > erichkeane wrote: > > cor3ntin wrote: > > > erichkeane wrot

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16806 +bool InTemplateDefinition = +getLangOpts().CPlusPlus && CurContext->isDependentContext(); + cor3ntin wrote: > erichkeane wrote: > > CplusPlus check is now not really b

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

erichkeane accepted this revision. erichkeane added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16806 +bool InTemplateDefinition = +getLangOpts().CPlusPlus && CurContext->isDependentContext(); +

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

erichkeane added a comment. In D144285#4135775 , @Endill wrote: > Thank you for the patch. > Any plans to backport this to 16.x branch? I would not really want us to do that, the breaking change here is concerning, and I'd like this to spend time in tru

[PATCH] D144285: [Clang] Implement CWG2518 - static_assert(false)

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:16805 +// definition, the declaration has no effect. +bool InTemplateDefinition = getLangOpts().CPlusPlus && getTemplateDepth(getCurScope()) != 0; + Hmm... interesting way to ca

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