[PATCH] D63954: Add lifetime categories attributes

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from a minor formatting nit. Thank you for working on these attributes! Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2517 +def err_a

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, though please remove the changes to `Conversion` if they're not needed before committing. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64666/new/ https://reviews

[PATCH] D64454: [clang-tidy] Adding static analyzer check to list of clang-tidy checks

aaron.ballman added a comment. In D64454#1598043 , @Nathan-Huckleberry wrote: > In D64454#1587102 , @aaron.ballman > wrote: > > > I think this looks reasonable to me, though I am still not certain if the > > rela

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

aaron.ballman added a comment. In D64838#1597771 , @xbolva00 wrote: > >> they parse the attributes first then attempt to parse a declaration; if > >> that fails, they fall back to parsing a statement > > Well, I don’t think this reparsing is ideal in te

[PATCH] D64914: Implement P1771

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from a minor naming nit. Comment at: clang/lib/Sema/SemaStmt.cpp:201 + SourceLocation Loc, SourceRange R1, +

[PATCH] D65279: [clang] Fail for empty names in is*DerivedFrom matchers.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, thank you for this! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65279/new/ https://reviews.llvm.org/D65279 ___

[PATCH] D64454: [clang-tidy] Adding static analyzer check to list of clang-tidy checks

aaron.ballman added a comment. In D64454#1601439 , @nickdesaulniers wrote: > In D64454#1600922 , @aaron.ballman > wrote: > > > I use svn in-tree, so I tried it out and it does not seem to work for me. > > > > c:

[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function

aaron.ballman marked an inline comment as done. aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:1172 return ExprError(); +LLVM_FALLTHROUGH; + case Builtin::BI__builtin_alloca: ziyig wrote: > aaron.ballman wrote: > > Do

[PATCH] D64883: Add new warning -Walloca for use of builtin alloca function

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, thank you for the patch! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64883/new/ https://reviews.llvm.org/D64883 __

[PATCH] D64380: Add 'require_designated_init' and 'required' attribute to clang

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Attr.td:1948 +def RequiresDesignator : InheritableAttr { + let Spellings = [Clang<"requires_designator">, GCC<"designated_init">]; + let Subjects = SubjectList<[Record]>; Why does this h

[PATCH] D63276: [AST] Add FunctionDecl::getParametersSourceRange()

aaron.ballman added inline comments. Comment at: clang/include/clang/AST/Decl.h:1928 + /// macro. Otherwise, it returns the location of the end of the ellipsis. + SourceRange getEllipsisSourceRange() const { +const auto *FPT = getType()->getAs(); Why a sour

[PATCH] D64454: [clang-tidy] Adding static analyzer check to list of clang-tidy checks

aaron.ballman added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/gen-static-analyzer-docs.py:113 + file_path_help = ("Path to Checkers.td, defaults to ../../../../clang/include/clang/StaticAnalyzer/Checkers/") + parse.add_argument("file", type=str, he

[PATCH] D64454: [clang-tidy] Adding static analyzer check to list of clang-tidy checks

aaron.ballman added a comment. When I run the script locally, I still get issues with `subprocess.run()`. c:\llvm\tools\clang\tools\extra\docs\clang-tidy\checks>python gen-static-analyzer-docs.py Traceback (most recent call last): File "gen-static-analyzer-docs.py", line 148, in m

[PATCH] D65308: [NFC][clang] Refactor getCompilationPhases()+Types.def step 3.

aaron.ballman accepted this revision. aaron.ballman 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/D65308/new/ https://reviews.llvm.org/D65308

[PATCH] D63082: [Diagnostics] Added support for -Wint-in-bool-context

aaron.ballman added inline comments. Comment at: test/SemaCXX/warn-int-in-bool-context.cpp:99 + + if (ans == yes || no) // expected-warning {{enum constant in boolean context}} +return a; Why do we want to diagnose this case in C++ but not in C? CHANGES S

[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

aaron.ballman added a comment. In D63139#1603667 , @xbolva00 wrote: > Ping again > > Is ‘CaseStmt´ AST C++ issue is a blocker for this patch or not? I do not think it is a blocker for this patch. It's the existing behavior of the AST and while annoying,

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:11656 + IsListInit = + IsListInit || (isa(OrigE) && S.getLangOpts().CPlusPlus); + Do you want to perform the `isa<>` on `OrigE` or on `E` which has had paren and implicit cast

[PATCH] D63276: [AST] Add FunctionDecl::getParametersSourceRange()

aaron.ballman added a comment. Phab looked to be having disk issues when I added my comments, so I'm pinging this review just to be sure the latest comments make it to the mailing list. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63276/new/ https://reviews.llvm.org/D63276

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

aaron.ballman added inline comments. Comment at: clang/lib/Parse/ParseStmt.cpp:103 MaybeParseCXX11Attributes(Attrs, nullptr, /*MightBeObjCMessageSend*/ true); + if (!MaybeParseOpenCLUnrollHintAttribute(Attrs)) Spurious newline? Comment at

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM! Comment at: clang/lib/Sema/SemaChecking.cpp:11656 + IsListInit = + IsListInit || (isa(OrigE) && S.getLangOpts().CPlusPlus); + ziangwan wrote: > aaron.ballman wrote: > > Do you want

[PATCH] D65050: [SemaTemplate] Mark a function type as dependent when its parameter list contains pack expansion

aaron.ballman added a comment. In D65050#1598057 , @cpplearner wrote: > In D65050#1596514 , @efriedma wrote: > > > Is this the only place where a compound type can contain a > > PackExpansionType? > > > Yes AFAIK.

[PATCH] D65206: [analyzer] Fix text range end columns in SARIF to be exclusive

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. This LGTM, but I'd appreciate a second reviewer chiming in only because Joe is a coworker. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D65209: [analyzer] Fix a SARIF exporter crash with macro expansions

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. This LGTM, but I'd appreciate a second reviewer chiming in only because Joe is a coworker. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D65211: [analyzer] Update the SARIF exporter to SARIF 2.1

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. This LGTM, but I'd appreciate a second reviewer chiming in only because Joe is a coworker. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llv

[PATCH] D64666: [Sema] Enable -Wimplicit-float-conversion for integral to floating point precision loss

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaChecking.cpp:11656 + IsListInit = + IsListInit || (isa(OrigE) && S.getLangOpts().CPlusPlus); + ziangwan wrote: > aaron.ballman wrote: > > ziangwan wrote: > > > aaron.ballman wrote: > > > >

[PATCH] D63082: [Diagnostics] Added support for -Wint-in-bool-context

aaron.ballman added inline comments. Comment at: test/Sema/warn-int-in-bool-context.c:26 + r = a << 7; // expected-warning {{'<<' in boolean context; did you mean '<'?}} + r = ONE << b; // expected-warning {{'<<' in boolean context; did you mean '<'?}} + jf

[PATCH] D65050: [SemaTemplate] Mark a function type as dependent when its parameter list contains pack expansion

aaron.ballman added a comment. I think this is reasonable, but I'd like @rsmith to weigh in -- this looks like it could be a Core Issue. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65050/new/ https://reviews.llvm.org/D65050 ___ cfe-commi

[PATCH] D65192: [Sema] Disable some enabled-by-default -Wparentheses diagnostics

aaron.ballman added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5654-5656 def warn_addition_in_bitshift : Warning< "operator '%0' has lower precedence than '%1'; " + "'%1' will be evaluated first">, InGroup, DefaultIgnore; MaskRay

[PATCH] D65192: [Sema] Disable some enabled-by-default -Wparentheses diagnostics

aaron.ballman added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5654-5656 def warn_addition_in_bitshift : Warning< "operator '%0' has lower precedence than '%1'; " + "'%1' will be evaluated first">, InGroup, DefaultIgnore; MaskRay

[PATCH] D64454: [clang-tidy] Adding static analyzer check to list of clang-tidy checks

aaron.ballman added a comment. This seems reasonable to me except for the fact that the script doesn't run when I try it locally. c:\llvm\tools\clang\tools\extra\docs\clang-tidy\checks>python gen-static-analyzer-docs.py Traceback (most recent call last): File "gen-static-analyzer-docs.p

[PATCH] D64380: Add 'require_designated_init' and 'required' attribute to clang

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Attr.td:1948 +def RequiresDesignator : InheritableAttr { + let Spellings = [Clang<"requires_designator">, GCC<"designated_init">]; + let Subjects = SubjectList<[Record]>; emmettneyman wr

[PATCH] D64454: [clang-tidy] Adding static analyzer check to list of clang-tidy checks

aaron.ballman accepted this revision. aaron.ballman 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/D64454/new/ https://reviews.llvm.org/D64454

[PATCH] D48866: [clang-tidy] Add incorrect-pointer-cast checker

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/IncorrectPointerCastCheck.cpp:69-70 + "cast from %0 to %1 may lead to access memory based on invalid " + "memory layout; pointed to type is strictly aligned than the " +

[PATCH] D65192: [Sema] Make -Wbitwise-op-parentheses and -Wlogical-op-parentheses disabled-by-default

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Thank you for this! LGTM aside from a testing request (doesn't require further review). Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5645 def warn_b

[PATCH] D62829: [clang-tidy] Check for dynamically initialized statics in headers.

aaron.ballman added inline comments. Comment at: clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp:33-35 + // This may work fine when optimization is enabled because bar() can + // be turned into a constant 7. But without optimization, it can + // ca

[PATCH] D62829: [clang-tidy] Check for dynamically initialized statics in headers.

aaron.ballman added inline comments. Comment at: clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp:33-35 + // This may work fine when optimization is enabled because bar() can + // be turned into a constant 7. But without optimization, it can + // ca

[PATCH] D65706: [docs] Better documentation for -Weverything

aaron.ballman added a comment. Thank you for working on this! Comment at: clang/docs/UsersManual.rst:998 +:option:`-Werror`, and also includes the warnings from :option:`-pedantic`. Some +diagnostics contradict each other, users of :option:`-Weverything` therefore +often disab

[PATCH] D65665: Support for attributes on @class declarations

aaron.ballman added a comment. I don't think I know enough about ObjC semantics to say much about the language design aspects of this patch. Comment at: clang/include/clang/Parse/Parser.h:1496 + DeclGroupPtrTy ParseObjCAtClassDeclaration(SourceLocation atLoc, +

[PATCH] D62829: [clang-tidy] Check for dynamically initialized statics in headers.

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:47 +void DynamicStaticInitializersCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus || getLangOpts().ThreadsafeStatics) +ret

[PATCH] D65706: [docs] Better documentation for -Weverything

aaron.ballman added inline comments. Comment at: clang/docs/UsersManual.rst:999-1000 +diagnostics contradict each other, users of :option:`-Weverything` therefore +often disable many diagnostics such as :option:`-Wno-c++98-compat` +:option:`-Wno-c++-compat`. + jf

[PATCH] D65706: [docs] Better documentation for -Weverything

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Comment at: clang/docs/UsersManual.rst:999-1000 +diagnostics contradict each other, users of :option:`-Weverything` therefore +often disable many diagnost

[PATCH] D62829: [clang-tidy] Check for dynamically initialized statics in headers.

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/DynamicStaticInitializersCheck.cpp:47 +void DynamicStaticInitializersCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus || getLangOpts().ThreadsafeStatics) +ret

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

aaron.ballman added inline comments. Comment at: clang/lib/Parse/ParseDecl.cpp:1767 } return ParseSimpleDeclaration(Context, DeclEnd, attrs, true); Should this also be passed `DeclSpecStart`? Comment

[PATCH] D64838: [Attr] Support _attribute__ ((fallthrough))

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. This LGTM, but you should hold off a bit to commit -- @rsmith, do you have any concerns with this approach? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION ht

[PATCH] D60543: [clang] Update isDerivedFrom to support Objective-C classes 🔍

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM Comment at: clang/lib/ASTMatchers/ASTMatchFinder.cpp:820 + llvm::DenseMap> + CompatibleAliases; gribozavr wrote: > `unordered_set`? or `SmallPtrSet`? Repository: rG LLVM Github

[PATCH] D65917: [clang-tidy] Added check for the Google style guide's category method naming rule.

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp:44 +CheckFactories.registerCheck( +"google-objc-require-category-method-prefixes"); CheckFactories.registerCheck( Please keep this list in a

[PATCH] D65912: [clang-tidy] Add new check for math constants

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/misc/MathConstantsCheck.cpp:44-58 +{{"M_E"}, {"std::math::e"}, 2.7182818284590452354}, +{{"M_LOG2E"}, {"std::math::log2e"}, 1.4426950408889634074}, +{{"M_LOG10E"}, {"

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:40 + + diag(UserVarDecl->getLocation(), "var %0 is %1 but holds a register") + << UserVarDecl << *VarType; How about `variable %0 declare

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:53 + } + diag(UserVarDecl->getLocation(), "use '%0'", DiagnosticIDs::Note) + << Replacement dsanders wrote: > aaron.ballman wrote: > > I

[PATCH] D65994: Extended FPOptions with new attributes

aaron.ballman added a comment. In general, this seems reasonable, but is missing test code. Comment at: clang/include/clang/Basic/LangOptions.h:188 +/// Rounding to nearest, corresponds to "round.tonearest". +ToNearest, +/// Rounding toward -Inf, corresponds to "rou

[PATCH] D65997: Add options rounding and exceptions to pragma fp

aaron.ballman added inline comments. Comment at: clang/docs/LanguageExtensions.rst:3146 +The option ``rounding`` specifies rounding mode applied to floating point +operations. It may take one of the values: ``tonearest``, ``downward``, specifies rounding mode -

[PATCH] D65912: [clang-tidy] Add new check for math constants

aaron.ballman added a comment. In D65912#1621898 , @ZaMaZaN4iK wrote: > The main reason why I've created this differential - asking to you about > usefulness of this check for clang-tidy. I understand that there are a some > TODO and formatting issues -

[PATCH] D65993: [NFC][clang] Adding argument based Phase list filtering to getComplicationPhases

aaron.ballman added inline comments. Comment at: clang/lib/Driver/Driver.cpp:3220 - unsigned LastPLSize = 0; - for (auto &I : Inputs) { -types::ID InputType = I.first; -const Arg *InputArg = I.second; + { +Arg *FinalPhaseArg; Why do you need this

[PATCH] D65912: [clang-tidy] Add new check for math constants

aaron.ballman added a comment. In D65912#1622963 , @ZaMaZaN4iK wrote: > > One thing I would be interested in knowing is how often the check behaves > > when run over some large, real-world code bases. Does it catch any true > > positives? Does it have fa

[PATCH] D65912: [clang-tidy] Add new check for math constants

aaron.ballman added a comment. In D65912#1623076 , @ZaMaZaN4iK wrote: > @aaron.ballman Sure. I agree with you that epsilon should be configurable. I > think we can collect some statistics later. Now I am going to work on > implementation and tests. Later

[PATCH] D65912: [clang-tidy] Add new check for math constants

aaron.ballman added a comment. In D65912#1623293 , @ZaMaZaN4iK wrote: > > My point regarding statistics is that the check needs to pull its own > > weight -- if it doesn't find many true positives, it's not of much value to > > a broad community, or if i

[PATCH] D65912: [clang-tidy] Add new check for math constants

aaron.ballman added a comment. In D65912#1623393 , @Eugene.Zelenko wrote: > In D65912#1623375 , @ZaMaZaN4iK > wrote: > > > > ... then I have a script that runs clang-tidy over all the compilation > > > units in a

[PATCH] D66028: [ASTDump] Add is_anonymous to VisitCXXRecordDecl

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66028/new/ https://reviews.llvm.org/D66028 ___ cfe-commits mailing lis

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

aaron.ballman marked an inline comment as done. aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp:28-29 CheckFactories.registerCheck("llvm-include-order"); +CheckFactories.registerCheck( +"llvm-prefer-register-o

[PATCH] D55125: [clang-tidy] Fix a false positive in misc-redundant-expression check

aaron.ballman added inline comments. Comment at: clang-tidy/misc/RedundantExpressionCheck.cpp:621 +static bool isSameToken(const Token &T1, const Token &T2, +const SourceManager &SM) { `isSameRawIdentifierToken()` so that the name isn't

[PATCH] D63423: [Diagnostics] Diagnose misused xor as pow

aaron.ballman added inline comments. Comment at: include/clang/Basic/DiagnosticGroups.td:508 def Varargs : DiagGroup<"varargs">; +def XorUsedAsPow : DiagGroup<"xor-used-as-pow">; xbolva00 wrote: > jfb wrote: > > Does this match the naming that GCC ended up wit

[PATCH] D65993: [NFC][clang] Adding argument based Phase list filtering to getComplicationPhases

aaron.ballman added inline comments. Comment at: clang/lib/Driver/Driver.cpp:3326 +types::getCompilationPhases(*this, Args, InputType, PL); +if (!PL.size()) continue; `PL.empty()` instead Comment at: clang/lib/Driver/Types.cpp:11

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

aaron.ballman added a comment. Rather than adding a bit onto `Expr` to specify whether it's erroneous, have you considered making this a property of the type system by introducing an `ErrorExpr` AST node that other nodes can inherit from? I think that approach would work more naturally for thin

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

aaron.ballman added a comment. In D65591#1625154 , @riccibruno wrote: > It seems that these two options are not exactly the same right ? The > `ContainsError` bit is useful to quickly answer "Does this expression > contains an invalid sub-expression" wit

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

aaron.ballman added a comment. In D65591#1625228 , @ilya-biryukov wrote: > In D65591#1625183 , @aaron.ballman > wrote: > > > In D65591#1625154 , @riccibruno > > wrote: > >

[PATCH] D66100: Add __has_builtin support for builtin function-like type traits.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from some nits. Comment at: docs/LanguageExtensions.rst:65 + + ``__has_builtin`` should not be used to detect support for a built-in macro; + use `

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp:26 void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { +using readability::NamespaceCommentCheck; + I would rather use the fully-qu

[PATCH] D65993: [NFC][clang] Adding argument based Phase list filtering to getComplicationPhases

aaron.ballman accepted this revision. aaron.ballman 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/D65993/new/ https://reviews.llvm.org/D65993

[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

aaron.ballman added a comment. In D65591#1626638 , @ilya-biryukov wrote: > In D65591#1625744 , @aaron.ballman > wrote: > > > The problem is: those bits are not infinite and we only have a handful left > > until b

[PATCH] D65997: Add options rounding and exceptions to pragma fp

aaron.ballman added inline comments. Comment at: clang/lib/Parse/ParsePragma.cpp:2697 PP.Lex(Tok); +StringRef ExpectedArgumentText; +switch (*FlagKind) { sepavloff wrote: > aaron.ballman wrote: > > Same here. > This case is different from the above,

[PATCH] D66058: [NFC][clang] Move much of the argument handling code from Driver::BuildActions to Driver::handleArguments.

aaron.ballman added inline comments. Comment at: clang/include/clang/Driver/Driver.h:264 + const InputList &Inputs, ActionList &Actions, + llvm::opt::Arg *YcArg, llvm::opt::Arg *YuArg) const; + I think the presence of t

[PATCH] D66058: [NFC][clang] Move much of the argument handling code from Driver::BuildActions to Driver::handleArguments.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66058/new/ https://reviews.llvm.org/D66058 ___

[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp:26 void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { +using readability::NamespaceCommentCheck; + dsanders wrote: > aaron.ballman

[PATCH] D27165: Add format_dynamic_key_arg attribute to improve "-Wformat" warnings for functions that load the formatting string dynamically based on a key value

aaron.ballman accepted this revision. aaron.ballman added a comment. I like this approach much better! LGTM, aside from a minor nit. Comment at: clang/include/clang/Basic/IdentifierTable.h:754 + /// If this selector is the specific keyword selector described by Names. + bool

[PATCH] D66206: [CodeGen] Don't keep stale pointers to LoopInfos

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66206/new/ https://reviews.llvm.org/D66206 ___

[PATCH] D65997: Add options rounding and exceptions to pragma fp

aaron.ballman added a comment. This generally LGTM, but there are open questions on https://reviews.llvm.org/D66092#1625380 that I think need to be answered before this should be committed. Comment at: clang/lib/Parse/ParsePragma.cpp:2678 + auto *AnnotValue = new (PP.getPr

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

aaron.ballman added a comment. There was a request in the linked bug for some code archaeology to see why this behavior exists in the first place. What were the results of that? I'm not opposed to the patch, but I would like to understand why it behaves the way it does. I could imagine "confus

[PATCH] D66269: [clang-tidy] Migrate objc-forbidden-subclassing to use isDerivedFrom 🚛

aaron.ballman accepted this revision. aaron.ballman 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/D66269/new/ https://reviews.llvm.org/D66269

[PATCH] D66270: [clang-tidy] Migrate objc-super-self to use isDerivedFrom 🚛

aaron.ballman accepted this revision. aaron.ballman 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/D66270/new/ https://reviews.llvm.org/D66270

[PATCH] D65997: Add options rounding and exceptions to pragma fp

aaron.ballman added inline comments. Comment at: clang/lib/Parse/ParsePragma.cpp:2678 + auto *AnnotValue = new (PP.getPreprocessorAllocator()) TokFPAnnotValue; while (Tok.is(tok::identifier)) { sepavloff wrote: > aaron.ballman wrote: > > Might as well move

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

aaron.ballman added a comment. In D66186#1631921 , @Nathan-Huckleberry wrote: > In D66186#1630427 , @aaron.ballman > wrote: > > > There was a request in the linked bug for some code archaeology to see why > > thi

[PATCH] D63423: [Diagnostics] Diagnose misused xor as pow

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. This LGTM aside from a few nits, but please give a chance for other reviewers to weigh in on their comments. Comment at: lib/Sema/SemaExpr.cpp:11025-11031 + i

[PATCH] D66186: [Sema] Don't warn on printf('%hd', [char]) (PR41467)

aaron.ballman requested changes to this revision. aaron.ballman added a comment. In D66186#1632440 , @Nathan-Huckleberry wrote: > As far as I can tell this case was just overlooked. The original commit > adding this change > https://reviews.llvm.org/rG0

[PATCH] D66364: Diagnose use of _Thread_local as an extension when appropriate

aaron.ballman created this revision. aaron.ballman added a reviewer: rsmith. Herald added a project: clang. We currently accept use of _Thread_local in all C and C++ modes, but we do not issue a warning about it being an extension when used outside of C11 mode. This patch adds the warning and ad

[PATCH] D66364: Diagnose use of _Thread_local as an extension when appropriate

aaron.ballman added a comment. In D66364#1633775 , @rsmith wrote: > `_Thread_local` is a reserved identifier; we generally don't produce > extension warnings for uses of reserved identifiers. (Eg, there's no warning > for `_Atomic` in C++ or `_Bool` in C

[PATCH] D66206: [CodeGen] Don't keep stale pointers to LoopInfos

aaron.ballman closed this revision. aaron.ballman added a comment. I've commit for you in r369259, thank you for the patch! Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66206/new/ https://reviews.llvm.org/D66206 ___

[PATCH] D66361: Improve behavior in the case of stack exhaustion.

aaron.ballman added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:14 let Component = "Sema" in { -let CategoryName = "Semantic Issue" in { +def warn_stack_exhausted : Warning< + "stack nearly exhausted; compilation time may suffer, and " --

[PATCH] D66365: [clang-tidy] [readability-convert-member-functions-to-static] ignore functions that hide base class methods

aaron.ballman added a comment. I'd like some help in understanding the motivation for this change. The bug report says `But the intention behind this is clearly that there is a member function A::foo which derived classes overwrite.` and I'm not certain that's so clear. It's been my experience

[PATCH] D66397: [Diagnostics] Improve -Wxor-used-as-pow

aaron.ballman added a comment. In D66397#1635715 , @xbolva00 wrote: > I agree what @tkanis suggested and be silent if RHS is macro as real world > code shows it. Opinions? > > @jfb @aaron.ballman I suspect we can come up with examples where the macro on

[PATCH] D66397: [Diagnostics] Improve -Wxor-used-as-pow

aaron.ballman added a comment. In D66397#1635745 , @xbolva00 wrote: > I think it is too soon to jump and disable all macros. We still catch all > motivating cases, it found bugs in Chromium. I think it's too soon to disable any macros. The workaround fo

[PATCH] D66364: Diagnose use of _Thread_local as an extension when appropriate

aaron.ballman added reviewers: mclow.lists, ldionne. aaron.ballman added a comment. Herald added a subscriber: dexonsmith. In D66364#1635795 , @rsmith wrote: > In D66364#1633981 , @aaron.ballman > wrote: > > > My m

[PATCH] D62829: [clang-tidy] Check for dynamically initialized statics in headers.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Comment at: clang-tools-extra/test/clang-tidy/bugprone-dynamic-static-initializers.hpp:1 +// RUN: %check_clang_tidy %s bugprone-dynamic-static-initialize

[PATCH] D63423: [Diagnostics] Diagnose misused xor as pow

aaron.ballman added inline comments. Comment at: lib/Sema/SemaExpr.cpp:10929 +// Do not diagnose if xor keyword is used. +if (ExprStr.find("xor") != llvm::StringRef::npos) + return; xbolva00 wrote: > xbolva00 wrote: > > jfb wrote: > > > xbolva00 wrot

[PATCH] D63082: [Diagnostics] Added support for -Wint-in-bool-context

aaron.ballman added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5609 +def warn_mul_in_bool_context : Warning< + "'*' in bool context, maybe you mean '&&'?">, + InGroup; xbolva00 wrote: > aaron.ballman wrote: > > I would appreciate se

[PATCH] D63082: [Diagnostics] Added support for -Wint-in-bool-context

aaron.ballman added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5625 + "evaluate to 'true'">, + InGroup; + xbolva00 wrote: > aaron.ballman wrote: > > xbolva00 wrote: > > > aaron.ballman wrote: > > > > This one seems like it should be

[PATCH] D63276: [AST] Add FunctionDecl::getParametersSourceRange()

aaron.ballman added a reviewer: aaron.ballman. aaron.ballman added inline comments. Comment at: clang/include/clang/AST/Decl.h:2331 + /// Attempt to compute an informative source range covering the + /// function parameters. This omits the ellipsis of a variadic function. + So

[PATCH] D60455: [SYCL] Implement SYCL device code outlining

aaron.ballman added inline comments. Comment at: clang/test/SemaSYCL/device-attributes.cpp:3 + +[[clang::sycl_kernel]] int gv2 = 0; // expected-warning {{'sycl_kernel' attribute only applies to functions}} +__attribute((sycl_kernel)) int gv3 = 0; // expected-warning {{'sycl_kern

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp:41-42 void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { +CheckFactories.registerCheck( +"google-upgrade-googletest-case"); CheckF

[PATCH] D63845: [WIP] Create a clang attribute that lets users specify LLVM attributes

aaron.ballman added a reviewer: aaron.ballman. aaron.ballman added a comment. In D63845#1559995 , @lebedev.ri wrote: > What's the target use-case here? What can't be solved with normal attributes? > I wonder if this should go to cfe+llvm -dev lists first,

[PATCH] D63082: [Diagnostics] Added support for -Wint-in-bool-context

aaron.ballman added a comment. In terms of the code, I think this is ready to go. However, I'm still not happy with `'*' in boolean context` as a diagnostic. Perhaps appending something about the resulting value always being true|false would help most of these diagnostics be more obvious. `blah

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