[PATCH] D62636: Driver, IRGen: Set partitions on GlobalValues according to -fsymbol-partition flag.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/include/clang/Driver/Options.td:938 HelpText<"Disable C++ static destructor registration">; +def fsymbol_partition_EQ : Joined<["-"],

[PATCH] D62009: [clang] perform semantic checking in constant context

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks, looks great. Comment at: clang/lib/Sema/SemaChecking.cpp:13941 /// +/// \param isConstantEvaluated wether the evalaution should be permormed in +/// constant

[PATCH] D62399: [clang] Add storage for APValue in ConstantExpr

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks like a good first step, thanks! Comment at: clang/include/clang/AST/ASTContext.h:2821-2823 + /// Adds an APValue that will be destructed during the destruction of the

[PATCH] D61790: [C++20] add Basic consteval specifier

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Only minor comments remain (other than the `-Wc++17-compat` warning). In the interest of incremental progress, let's leave the `-Wc++17-compat` warning for a later patch; feel free to commit

[PATCH] D61790: [C++20] add Basic consteval specifier

rsmith added a comment. Thank you! Comment at: clang/include/clang/AST/DeclBase.h:1497 + +/// kind of Contexpr specifier as defined by ConstexprSpecKind. +uint64_t ConstexprKind : 2; "kind" -> "Kind" "Contexpr" -> "constexpr" Comment

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

rsmith added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:5447 + +case APValue::LValue: { + LValue LVal; jfb wrote: > rsmith wrote: > > This will permit bitcasts from `nullptr_t`, providing a zero value. I think > > that's wrong; the bit

[PATCH] D62825: [C++2a] Add __builtin_bit_cast, used to implement std::bit_cast

rsmith added a comment. Can we also use the same bit reader/writer implementation to generalize the current implementation of `__builtin_memcpy` and friends? (I don't think we can remove the existing implementation, sadly, since we allow copying arrays of the same trivially-copyable type today

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

rsmith added inline comments. Comment at: lib/Sema/SemaExprCXX.cpp:7762-7787 // Ensure none of the TypoExprs have multiple typo correction candidates // with the same edit length that pass all the checks and filters. // TODO: Properly handle various permutations

[PATCH] D62550: [coroutines][PR41909] Don't build dependent coroutine statements for generic lambda

rsmith added inline comments. Comment at: cfe/trunk/lib/Sema/TreeTransform.h:7173 +auto *MD = dyn_cast_or_null(FD); +if (!MD || !MD->getParent()->isGenericLambda()) { + assert(!Promise->getType()->isDependentType() && This assert doesn't seem

[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

rsmith added inline comments. Comment at: lib/Sema/SemaExprCXX.cpp:7713-7714 + // Add the newly created typos to the TypoExprs list, even if they + // failed to apply. This allows them to be reaped although they won't + // emit any diagnostic. +

[PATCH] D62435: Add Attribute NoThrow as an Exception Specifier Type

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Seems fine to me; please wait for @aaron.ballman's review to conclude as well. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62435/new/ https://reviews.llvm.org/D62435

[PATCH] D62470: Clarify when fix-it hints on warnings are appropriate

rsmith added a comment. Thanks, looks good to me. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62470/new/ https://reviews.llvm.org/D62470 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D62429: Fix linkage of _ZTS strings for types involving incomplete classes to match the Itanium ABI rules.

rsmith added a comment. In D62429#1518602 , @EricWF wrote: > I think this is llvm.org/PR37398 > > I tried fixing this a while back in r332028 but the fix got reverted for > causing llvm.org/PR37545. Looks like I had a fix for that in

[PATCH] D62435: Add Attribute NoThrow as an Exception Specifier Type

rsmith requested changes to this revision. rsmith added a comment. This revision now requires changes to proceed. Seems like a nice approach to the problem, but we need to not break the libclang C ABI :) Comment at: clang/include/clang-c/Index.h:202-204 + * The cursor has a

[PATCH] D62429: Fix linkage of _ZTS strings for types involving incomplete classes to match the Itanium ABI rules.

rsmith created this revision. rsmith added a reviewer: rjmccall. Herald added a project: clang. Herald added a subscriber: cfe-commits. Without this patch, type_info objects for pointers to incomplete classes compare unequal with libc++ when formed in multiple translation units, because each

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

rsmith added a comment. In D59402#1516589 , @aaronpuchert wrote: > Perhaps we could clarify in the docs that fix-it hints on warnings are > appropriate only if they don't change anything except suppressing the warning. That sounds like a great idea.

[PATCH] D56571: [RFC prototype] Implementation of asm-goto support in clang

rsmith accepted this revision. rsmith marked an inline comment as done. rsmith added a comment. Looks good with a few largely-mechanical changes. Comment at: include/clang/Basic/DiagnosticParseKinds.td:30 "GNU-style inline assembly is disabled">; -def

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

rsmith added a comment. In D59402#1516479 , @aaronpuchert wrote: > In D59402#1516432 , @aaron.ballman > wrote: > > > Also, we're not attempting to recover from the error, which is a good point > > that @thakis

[PATCH] D59402: Fix-it hints for -Wmissing-{prototypes,variable-declarations}

rsmith added inline comments. Comment at: lib/Sema/SemaDecl.cpp:11810 + << var + << ((var->getStorageClass() != SC_Extern) + ? FixItHint::CreateInsertion(var->getBeginLoc(), "static ") It would be more appropriate to suppress

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

rsmith marked 2 inline comments as done. rsmith added inline comments. Comment at: clang/lib/Parse/ParseDecl.cpp:4321 + isCXXDeclarationSpecifier(ITC_Never, TPResult::True) != + TPResult::True) || +(!getLangOpts().CPlusPlus &&

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

rsmith added inline comments. Comment at: clang/include/clang/Sema/DeclSpec.h:1753-1758 +// Describes whether the current context is a context where an implicit +// typename is allowed (C++2a [temp.res]p5]). +enum ImplicitTypenameContext { + ITC_Never, + ITC_Yes, +};

[PATCH] D62009: [clang] perform semantic checking in constant context

rsmith added a comment. Interesting. I think all of the new warnings in the test cases here are undesirable (they duplicate errors produced by the constant evaluator), but the removed warnings all look like improvements. Generally, I think our goals should be: For code patterns that might

[PATCH] D62009: [clang] perform semantic checking in constant context

rsmith added a comment. In D62009#1506415 , @Tyker wrote: > there are issues with using ConstantExpr to mark expressions as constant for > semantic checking: > > - #1 multpile Expr::Ignore* operation remove ConstantExpr from the expression. `Ignore*`

[PATCH] D61722: [AST] Add RecoveryExpr; produce it on overload resolution failure and missing member.

rsmith added a comment. In D61722#1504376 , @sammccall wrote: > In D61722#1503863 , @rsmith wrote: > > > I expect we'll want a `ContainsErrors` bit on `Stmt`, propagated similarly > > to the existing

[PATCH] D62116: [Sema] raise nullptr check to cover additional uses

rsmith requested changes to this revision. rsmith added inline comments. This revision now requires changes to proceed. Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:557-558 +dyn_cast_or_null(ND->getDeclContext()); +CXXThisScopeRAII

[PATCH] D61838: [Sema] Suppress additional warnings for C's zero initializer

rsmith added inline comments. Comment at: clang/lib/AST/Expr.cpp:2092-2096 const IntegerLiteral *Lit = dyn_cast(getInit(0)); + if (!Lit) { +if (const ImplicitCastExpr *ICE = dyn_cast(getInit(0))) + Lit = dyn_cast(ICE->getSubExpr()); + } Use

[PATCH] D61522: Added an assertion to constant evaluation enty points that prohibits dependent expressions

rsmith accepted this revision. rsmith added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:6369 // very difficult. Ideally, we should handle them more gracefully. -if (!EIA->getCond()->EvaluateWithSubstitution( +if (EIA->getCond()->isValueDependent()

[PATCH] D62009: [clang] perform semantic checking in constant context

rsmith added a comment. In D62009#1505263 , @Tyker wrote: > i added this bit because when we eventually store the result of evaluation in > ConstantExpr we will probably want to trail-allocate the APValue's possible > representations separately to limit

[PATCH] D62009: [clang] perform semantic checking in constant context

rsmith added a comment. The approach we've been taking for this up until now is to use a `ConstantExpr` node to mark the entry point of a constant context; it looks like that would continue to work here, but we're missing those nodes in some cases? (This is preferable to using a flag because

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

rsmith accepted this revision. rsmith marked an inline comment as done. rsmith added a comment. This revision is now accepted and ready to land. Thanks, LGTM! Comment at: clang/lib/Lex/Pragma.cpp:370 // Push the tokens onto the stack. - EnterTokenStream(TokArray,

[PATCH] D36357: Added a better diagnostic when using the delete operator with lambdas

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

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

rsmith added a comment. In D58920#1503872 , @modocache wrote: > Thanks for the help, @rsmith! Your suggestions were spot-on. (It took me a > little while to figure out why, even using the `LazyDeclPtr` directly, I was > still triggering deserialization.

[PATCH] D61722: [AST] Add RecoveryExpr; produce it on overload resolution failure and missing member.

rsmith added a comment. I expect we'll want a `ContainsErrors` bit on `Stmt`, propagated similarly to the existing `InstantiationDependent` / `ContainsUnexpandedParameterPack` / ... bits. For example, the constant evaluator will want to quickly bail out of evaluating expressions and statements

[PATCH] D61756: Add a __FILE_NAME__ macro.

rsmith added a subscriber: alexr. rsmith added a comment. Generally, I think this is a good, useful feature, but there's one point from http://clang.llvm.org/get_involved.html 's checklist for accepting language extensions on which its case is weak, as reflected by this feedback from @alexr on

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

rsmith added inline comments. Comment at: clang/lib/Lex/PPDirectives.cpp:1521 Tok[0].setAnnotationValue(AnnotationVal); - EnterTokenStream(std::move(Tok), 1, true); + EnterTokenStream(std::move(Tok), 1, true, /*IsReinject*/ true); } I think it'd be more

[PATCH] D36357: Added a better diagnostic when using the delete operator with lambdas

rsmith added a comment. In D36357#1501999 , @Rakete wrote: > In D36357#1501961 , @rsmith wrote: > > > In D36357#1500949 , @Rakete > > wrote: > > > > > How should I

[PATCH] D36357: Added a better diagnostic when using the delete operator with lambdas

rsmith added a comment. In D36357#1500949 , @Rakete wrote: > How should I do this? Do I just skip to the next `}`, or also take into > account any additional scopes? Also does this mean that I skip and then > revert, because that seems pretty

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

rsmith added a comment. In D59885#1501774 , @ilya-biryukov wrote: > The suggested approach looks promising. The difference seems to be within the > noise levels on my machine: :) Awesome! > I guess it would make more sense to do the following before

[PATCH] D61790: [C++20] add consteval specifier

rsmith added a comment. Thanks for working on this! Comments on the general approach: - We should only evaluate each immediate invocation once (this will become essential once we start supporting reflection -- and particularly operations that mutate the AST -- inside `consteval` functions). -

[PATCH] D61370: Add a C2x mode and allow attributes in it

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: lib/Frontend/CompilerInvocation.cpp:2593-2596 + // Enable [[]] attributes in C++11 and C2x by default. + Opts.DoubleSquareBracketAttributes = Args.hasFlag(

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

rsmith added a comment. The problem may well be that you're recursively triggering deserialization of the `std` namespace from its own deserialization. In D58920#1500246 , @modocache wrote: > @@ -3401,6 +3402,22 @@ ASTDeclReader::FindExistingResult >

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

rsmith added a comment. Thinking about this some more: distinguishing between "macro expansion" and other cases seems like a proxy for "this token came from inside the preprocessor / lexer" versus "this token was provided by the user", which is also exactly what `IsNewToken` is supposed to

[PATCH] D61858: Make `__is_base_of` more friendly with unions

rsmith accepted this revision. rsmith added a comment. Yeah, this seems to match the library wording. (I think it's short-sighted and over-fitting -- this is baking into the library specification that the language happens to not allow unions to have base classes today -- but this isn't the

[PATCH] D61809: [BPF] Preserve original struct/union type name/access index and array subscripts

rsmith added a comment. If I understand correctly, you want to be able to compile a program against some `struct` and `union` layouts, and then at load time "update" the program to cope with the actual layouts for those types being something else, but containing (at least) the set of members

[PATCH] D43576: Solution to fix PR27066 - Redefinition with same mangled name as another definition (dllexport and uuid)

rsmith added inline comments. Comment at: include/clang/AST/Decl.h:4281 + class DeclSpecUuidDecl : public Decl { + StringRef StrUuid; + public: Can we store a `StringLiteral*` here instead? Comment at: include/clang/Sema/Sema.h:393-394 +

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

rsmith added a comment. In D59885#1496734 , @ilya-biryukov wrote: > Overall, the performance cost in empty-callback case seems to be lower than > `5%`. This is significant, but hopefully acceptable. 5% is a lot to pay for something that we won't be

[PATCH] D61717: Fix arm_neon.h to be clean under -fno-lax-vector-conversions.

rsmith updated this revision to Diff 198762. rsmith added a comment. Remove unneeded test change. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61717/new/ https://reviews.llvm.org/D61717 Files: test/Headers/arm-neon-header.c utils/TableGen/NeonEmitter.cpp

[PATCH] D61717: Fix arm_neon.h to be clean under -fno-lax-vector-conversions.

rsmith created this revision. rsmith added reviewers: t.p.northover, javed.absar, SjoerdMeijer, pbarrio. Herald added a subscriber: kristof.beyls. Herald added a project: clang. arm_neon.h produces a large number of errors under -fno-lax-vector-conversions, which disables some deeply dangerous

[PATCH] D61522: Added an assertion to constant evaluation enty points that prohibits dependent expressions

rsmith added inline comments. Comment at: clang/lib/Sema/SemaOverload.cpp:6369 // very difficult. Ideally, we should handle them more gracefully. -if (!EIA->getCond()->EvaluateWithSubstitution( +if (EIA->getCond()->isValueDependent() || +

[PATCH] D60910: [WIP] Dumping the AST to JSON

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. If you're happy with these two conditions, then I have no concerns with this moving forward: - There is no implied stability for the content or format of the dump between major releases,

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

rsmith added inline comments. Comment at: clang/lib/Lex/Preprocessor.cpp:870-900 + TokenSource Source; do { +Source = TokenSource(); + switch (CurLexerKind) { case CLK_Lexer: + Source.InDirective = CurLexer->ParsingPreprocessorDirective;

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

rsmith added inline comments. Comment at: clang/lib/Sema/SemaDeclCXX.cpp:13121-13122 + if (VD->isNoDestroy(getASTContext()) && + !(VD->getType()->isArrayType() && getLangOpts().Exceptions && +VD->isStaticLocal())) return; Hmm, perhaps it would

[PATCH] D61522: Added an assertion to constant evaluation enty points that prohibits dependent expressions

rsmith added a comment. The right thing to check in all of these cases should be only `isValueDependent()`. Every type-dependent expression should generally also be value-dependent (because the type is part of the value), but value-dependent exactly means "dependent in a way that prevents

[PATCH] D61396: [hip] Fix ambiguity from `>>>` of CUDA.

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Parse/ParseTentative.cpp:597 NextToken().isOneOf(tok::greater, tok::greatergreater, +

[PATCH] D61656: -frewrite-imports: Add support for wildcard rules in umbrella modules with

rsmith accepted this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: lib/Basic/Module.cpp:324 +Module *Module::findOrCreateSubmodule(StringRef Name) { + llvm::StringMap::const_iterator Pos = SubModuleIndex.find(Name);

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

rsmith added a comment. In D61165#1492830 , @rjmccall wrote: > In D61165#1492781 , @rsmith wrote: > > > (Those destructor calls are separate full-expressions that happen > > afterwards; see [intro.execution]/5.) >

[PATCH] D61396: [hip] Fix ambiguity from `>>>` of CUDA.

rsmith added inline comments. Comment at: clang/lib/Parse/ParseTentative.cpp:597 + tok::comma || +(getLangOpts().CUDA && Tok.is(tok::greatergreatergreater { TPR = TPResult::True; No need to

[PATCH] D61165: Fix a crash where a [[no_destroy]] destructor was not emitted in an array

rsmith added a comment. In D61165#1490417 , @erik.pilkington wrote: > In D61165#1490168 , @rjmccall wrote: > > > I think the intuitive rule is that initialization is complete when the > > full-expression

[PATCH] D60934: [clang] adding explicit(bool) from c++2a

rsmith reopened this revision. rsmith added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:373-374 + ExplicitSpecifier ES, FunctionDecl *New) { + if (!ES.getExpr() ||

[PATCH] D36357: Added a better diagnostic when using the delete operator with lambdas

rsmith added a comment. In D36357#1491081 , @Rakete wrote: > @rsmith One last question: The fixit diagnostic seems to be inconsistent with > the rest? > > main.cpp:2:3: error: '[]' after delete interpreted as 'delete[]' > delete[] { return new

[PATCH] D61552: [clang] Adapt ASTMatcher to explicit(bool) specifier

rsmith added a comment. This seems reasonable to me, but I'll leave it to @klimek or someone else to judge whether the `isExplicit` / `hasExplicitSpecifier` approach is the right way to expose this functionality to matcher users. Repository: rC Clang CHANGES SINCE LAST ACTION

[PATCH] D40381: Parse concept definition

rsmith accepted this revision. rsmith added a comment. LGTM with a few mechanical updates. Comment at: include/clang/Basic/DiagnosticParseKinds.td:1262 + "name defined in concept definition must be an identifier">; +def err_concept_legacy_bool_keyword : ExtWarn< + "ISO C++2a

[PATCH] D36357: Added a better diagnostic when using the delete operator with lambdas

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks! Some minor nits, please feel free to commit once they're addressed. In D36357#1177852 , @Rakete wrote: > Note that clang doesn't support

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

rsmith added inline comments. Comment at: clang/include/clang/Sema/Sema.h:1784 bool IsClassTemplateDeductionContext = true, + bool AllowImplicitTypename = false, IdentifierInfo **CorrectedII = nullptr);

[PATCH] D36527: Implemented P0428R2 - Familiar template syntax for generic lambdas

rsmith accepted this revision. rsmith marked an inline comment as done. rsmith added a comment. This revision is now accepted and ready to land. Thank you, please go ahead and land this! (We'll need to parse and handle //requires-clause//s here too, but that can wait for another patch.) (Please

[PATCH] D61506: [OpenCL] Switch to C++17

rsmith added a comment. Per the OpenCL C++ 1.0 specification, section 2: > The OpenCL C++ programming language is based on the ISO/IEC JTC1 SC22 WG21 N > 3690 language specification (a.k.a. C++14 specification). I think it would be reasonable to permit changing the base C++ standard in OpenCL

[PATCH] D61357: SemaOverload: Complete candidates before emitting the error, to ensure diagnostics emitted (or suppressed) during completion don't interfere with the overload notes

rsmith added a comment. In D61357#1485581 , @dblaikie wrote: > Oh, @rsmith - if there's any better/different testing (if you can figure out > how to reduce the test case down further, now that we know the cause - or if > you'd like testing for other

[PATCH] D61357: SemaOverload: Complete candidates before emitting the error, to ensure diagnostics emitted (or suppressed) during completion don't interfere with the overload notes

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM, thanks! Comment at: lib/Sema/SemaOverload.cpp:3518-3519 << false << From->getType() << From->getSourceRange() << ToType; } else return false; +

[PATCH] D60934: [clang] adding explicit(bool) from c++2a

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Looks good, thank you! Comment at: clang/lib/Sema/SemaInit.cpp:9359 // The converting constructors of T are candidate functions. if (Kind.isCopyInit() &&

[PATCH] D51329: [Attribute/Diagnostics] Print macro if definition is an attribute declaration

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Thanks, looks great! I think the handling of the expansion location in the `MacroQualifiedTypeLoc` isn't quite right yet (it looks like it will never actually be set at all, because the code

[PATCH] D60934: [clang] adding explicit(bool) from c++2a

rsmith added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:9361 // Only consider converting constructors. -if (GD->isExplicit()) +if (!GD->isMaybeNotExplicit()) continue; Tyker wrote: > rsmith wrote: > > Tyker

[PATCH] D60934: [clang] adding explicit(bool) from c++2a

rsmith added a comment. This is great, thank you so much! Comment at: clang/include/clang/AST/DeclBase.h:1539-1541 +uint64_t NumCtorInitializers : 64 - NumDeclContextBits - +NumFunctionDeclBits - +/*Other used bits in CXXConstructorDecl*/ 3;

[PATCH] D51329: [Attribute/Diagnostics] Print macro if definition is an attribute declaration

rsmith added inline comments. Comment at: clang/lib/Parse/ParseDecl.cpp:257-262 + bool AttrStartIsInMacro = + (StartLoc.isMacroID() && Lexer::isAtStartOfMacroExpansion( + StartLoc, SrcMgr, PP.getLangOpts())); + bool

[PATCH] D59885: [Lex] Allow to consume tokens while preprocessing

rsmith added a comment. I'd like to understand more about the intended use cases of this functionality. What information do clients want? Comment at: clang/lib/Lex/Preprocessor.cpp:870-900 + TokenSource Source; do { +Source = TokenSource(); + switch

[PATCH] D60934: [clang] adding explicit(bool) from c++2a

rsmith marked 2 inline comments as done. rsmith added a comment. Thanks, this is looking really good. (Lots of comments but they're mostly mechanical at this point.) Comment at: clang/include/clang/AST/DeclBase.h:1539-1541 +uint64_t NumCtorInitializers : 64 -

[PATCH] D51329: [Attribute/Diagnostics] Print macro if definition is an attribute declaration

rsmith added inline comments. Herald added a subscriber: rnkovacs. Comment at: clang/include/clang/AST/Type.h:4174 +/// Sugar type that represents a type that was defined in a macro. +class MacroQualifiedType : public Type { This comment is out of date. "[...]

[PATCH] D58841: [Diagnostics] Support -Wtype-limits for GCC compatibility

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. In D58841#1483128 , @xbolva00 wrote: > Thanks for the review! If the patch is fine, please approve it. Sure thing! (Phab doesn't permit approving a

[PATCH] D58841: [Diagnostics] Support -Wtype-limits for GCC compatibility

rsmith added a comment. > xbolva00 abandoned this revision. Do you not want to pursue this any more? This seems reasonable to me, and is in line with other cases where we have diagnostic flags as aliases to GCC's similar-but-not-quite-the-same flags (eg, GCC's `-Wnoexcept-type` doesn't fire

[PATCH] D60943: Delay diagnosing "n" constraint until after inlining

rsmith added a comment. In D60943#1475038 , @void wrote: > In D60943#1474926 , @rsmith wrote: > > > Is `"n"` really special in this regard, or is it just the first one that > > we've encountered? > > > I think

[PATCH] D52521: [Sema] DR727: Ensure we pick up enclosing template instantiation arguments for a class-scope explicit specialization.

rsmith added a comment. In D52521#1479907 , @erik.pilkington wrote: > Looks like @rsmith fixed this in r359266. Sorry, I completely forgot about the existence of this when working on that patch! :( CHANGES SINCE LAST ACTION

[PATCH] D37035: Implement __builtin_LINE() et. al. to support source location capture.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Herald added a subscriber: rnkovacs. Some cleanups and simplifications, but LGTM with those applied. Thank you! Comment at: docs/LanguageExtensions.rst:2335 + +When the

[PATCH] D48292: use modern type trait implementations when available

rsmith added a comment. In D48292#1478617 , @ldionne wrote: > I think this broke the C++03 bots: > http://green.lab.llvm.org/green/view/Libcxx/job/libc++%20and%20libc++abi%20trunk/CI_ARCH=64,CI_EXCEPTIONS=ON,CI_STD=c++03/103/consoleFull Thanks, should

[PATCH] D60934: [clang] adding explicit(bool) from c++2a

rsmith marked an inline comment as done. rsmith added inline comments. Comment at: clang/include/clang/AST/DeclCXX.h:2022 + + bool isEquivalentExplicit(const CXXDeductionGuideDecl *Other) const; + bool isExplicit() const { I think this is too special-case to

[PATCH] D48292: use modern type trait implementations when available

rsmith marked an inline comment as done. rsmith added inline comments. Comment at: include/type_traits:3683 + +#elif __has_feature(has_trivial_destructor) || (_GNUC_VER >= 403) + EricWF wrote: > rsmith wrote: > > EricWF wrote: > > > We don't support anything

[PATCH] D48292: use modern type trait implementations when available

This revision was automatically updated to reflect the committed changes. Closed by commit rCXX359159: Use modern type trait implementations when available. (authored by rsmith, committed by ). Changed prior to commit: https://reviews.llvm.org/D48292?vs=151761=196559#toc Repository: rCXX

[PATCH] D48292: use modern type trait implementations when available

rsmith marked an inline comment as done. rsmith added inline comments. Comment at: include/type_traits:3683 + +#elif __has_feature(has_trivial_destructor) || (_GNUC_VER >= 403) + EricWF wrote: > We don't support anything before GCC 4.9, so you can replace the

[PATCH] D60943: Delay diagnosing "n" constraint until after inlining

rsmith added a comment. In D60943#1474899 , @void wrote: > Here's the motivating bug report: https://bugs.llvm.org/show_bug.cgi?id=41027 Thanks, that's illuminating. OK, if we want to support that code, then there's really not much validation we can do

[PATCH] D55500: [Builtins] Implement __builtin_is_constant_evaluated for use in C++2a

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. Herald added a subscriber: dexonsmith. LGTM, thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55500/new/ https://reviews.llvm.org/D55500

[PATCH] D60943: Delay diagnosing "n" constraint until after inlining

rsmith added a comment. I don't like making the validity of an input depend on optimizations, but I presume this is necessary to build Linux or similar? I'd be more comfortable with this if we still did the eager check if the enclosing function doesn't have the `always_inline` attribute. Is

[PATCH] D59711: PR41183: Don't emit Wstrict-prototypes warning for an implicit function declaration.

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59711/new/ https://reviews.llvm.org/D59711

[PATCH] D60872: Add new warning knob for unknown attribute namespaces

rsmith added a comment. Hmm. So there are a few different situations where we might meet an unknown attribute (I'm sure I missed some): 1. The attribute had a typo in it (eg: [[clagn::fallthrough]]). 2. The attribute has semantic effects (ignoring it is incorrect and will -- at best -- not

[PATCH] D60892: Modules: Search for a visible definition of the decl context when computing visibility of a default template parameter

rsmith accepted this revision. rsmith added a comment. This revision is now accepted and ready to land. LGTM, with a possible optimization. Thank you! I know this bug was incredibly hard to track down. =) Comment at: lib/Sema/SemaLookup.cpp:1551 +

[PATCH] D41284: [Concepts] Associated constraints infrastructure.

rsmith added inline comments. Comment at: include/clang/AST/DeclTemplate.h:177 + /// conjunction ("and"). + llvm::SmallVector getAssociatedConstraints() const; + Returning a `SmallVector` by value is not idiomatic. If you need to make a copy here, you should

[PATCH] D44352: [Concepts] Constrained template parameters

rsmith requested changes to this revision. rsmith added a comment. This revision now requires changes to proceed. This needs revision to reflect changes between the Concepts TS and C++20. In particular, only the name of a //type-concept// can be used to declare a //template-parameter// in the

[PATCH] D60845: [VerifyDiagnosticConsumer] Document -verify= in doxygen

rsmith added a comment. In D60845#1471741 , @jdenny wrote: > In D60845#1471002 , @rsmith wrote: > > > I've seen a few projects outside of clang use `-verify` mode for their own > > testing of various things. > > >

[PATCH] D60845: [VerifyDiagnosticConsumer] Document -verify= in doxygen

rsmith added a comment. In D60845#1470986 , @NoQ wrote: > Yup, i confirm that this improves discoverability of this feature :) Maybe it > deserves its own .rst doc, like FileCheck > , but for now

[PATCH] D60826: Clarify -Winfinite-recursion message

rsmith requested changes to this revision. rsmith added inline comments. This revision now requires changes to proceed. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:64 def warn_infinite_recursive_function : Warning< - "all paths through this function will call

[PATCH] D59754: [Sema] Add c++2a designated initializer warnings

rsmith added a comment. I think you're missing the enforcement of the rule that the same field name cannot be designated multiple times in a single //designated-initializer-list//. I'm fine with that being done separately (not as part of this patch), though. Comment at:

[PATCH] D59754: [Sema] Add c++2a designated initializer warnings

rsmith added inline comments. Comment at: clang/lib/Sema/SemaInit.cpp:2017 + auto LastIdx = Field != FieldEnd + ? Field->getFieldIndex() Please use an actual type rather than `auto` here and below. Comment at:

[PATCH] D40381: Parse concept definition

rsmith added a comment. Thanks! Please revert the (presumably unintended) mode changes to the `ptxas` executables. Comment at: include/clang/AST/DeclTemplate.h:3035 + SourceRange getSourceRange() const override LLVM_READONLY { +return SourceRange(getLocation(),

<    9   10   11   12   13   14   15   16   17   18   >