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

aaron.ballman added inline comments. Comment at: lib/Sema/SemaExpr.cpp:11067 + + // Do not diagnose 2 ^ 64, but allow special case (2 ^ 64) - 1. + if (SubLHS && SubRHS && (LeftSideValue != 2 || RightSideValue != 64)) aaron.ballman wrote: > This comment explains

[PATCH] D66919: Warn about zero-parameter K&R definitions in -Wstrict-prototypes

aaron.ballman added a comment. In D66919#1655052 , @aaronpuchert wrote: > In D66919#1655048 , @dexonsmith > wrote: > > > I went back to read notes from when we deployed `-Wstrict-prototypes` > > (which we have ha

[PATCH] D67140: [analyzer][NFC] Fix inconsistent references to checkers as "checks"

aaron.ballman added a comment. In D67140#1658353 , @gribozavr wrote: > In D67140#1658315 , @aaron.ballman > wrote: > > > In D67140#1656831 , @NoQ wrote: > > > > > Honestly,

[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/RequireCategoryMethodPrefixesCheck.cpp:16 +namespace { +const char *kCustomCategoryMethodIdentifier = "ThisIsACategoryMethod"; +} // anonymous namespace Eugene.Zelenko wrote: >

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

aaron.ballman added inline comments. Comment at: lib/Sema/SemaExpr.cpp:11067 + + // Do not diagnose 2 ^ 64, but allow special case (2 ^ 64) - 1. + if (SubLHS && SubRHS && (LeftSideValue != 2 || RightSideValue != 64)) xbolva00 wrote: > xbolva00 wrote: > > aaron.

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

aaron.ballman added inline comments. Comment at: lib/Sema/SemaExpr.cpp:11067 + + // Do not diagnose 2 ^ 64, but allow special case (2 ^ 64) - 1. + if (SubLHS && SubRHS && (LeftSideValue != 2 || RightSideValue != 64)) jfb wrote: > xbolva00 wrote: > > xbolva00 wr

[PATCH] D64671: [clang-tidy] New check: misc-init-local-variables

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp:76 + else if (TypePtr->isFloatingType()) { +InitializationString = " = NAN"; +AddMathInclude = true; jpakkane wrote: > aaron.ballman w

[PATCH] D67159: [clang] New __attribute__((__clang_builtin)).

aaron.ballman added a comment. I share in the discomfort expressed for this attribute, but I don't have a better solution in mind just yet, so I'm giving some other review feedback in the meantime. Comment at: clang/include/clang/Basic/Attr.td:596 } +def ClangBuiltinOverride

[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/RequireCategoryMethodPrefixesCheck.cpp:30 +ClangTidyOptions::OptionMap &Opts) { + Options.store(Opts, "WhitelistedPrefixes", WhitelistedPrefixes); +} aaron.ballman wrote: >

[PATCH] D67159: [clang] New __attribute__((__clang_builtin)).

aaron.ballman added a comment. In D67159#1659103 , @simon_tatham wrote: > Come to think of it, it would also not be too hard to constrain it to > //only// be usable for a particular subset of builtins, and perhaps even only > with a particular set of al

[PATCH] D66919: Warn about zero-parameter K&R definitions in -Wstrict-prototypes

aaron.ballman added a comment. In D66919#1658483 , @aaronpuchert wrote: > In D66919#1658355 , @aaron.ballman > wrote: > > > We do have numerous warnings that are default errors, you can look for > > `DefaultError

[PATCH] D67140: [analyzer][NFC] Fix inconsistent references to checkers as "checks"

aaron.ballman added a comment. In D67140#1658969 , @gribozavr wrote: > In D67140#1658365 , @aaron.ballman > wrote: > > > Ah, good to know! That reduces my concern, but doesn't negate it. AFAIK, we > > haven't chan

[PATCH] D54408: [ASTMatchers] Add matchers available through casting to derived

aaron.ballman added a comment. In D54408#1659301 , @Abpostelnicu wrote: > @aaron.ballman > I think the auto usage improves and simplifies the code, however, as I would > really like to see this code landed, I would be ok to help Stephen to finish > th

[PATCH] D67140: [analyzer][NFC] Fix inconsistent references to checkers as "checks"

aaron.ballman added a comment. In D67140#1659749 , @gribozavr wrote: > In D67140#1659356 , @aaron.ballman > wrote: > > > In D67140#1658969 , @gribozavr > > wrote: > > > > >

[PATCH] D67140: [analyzer][NFC] Fix inconsistent references to checkers as "checks"

aaron.ballman added a comment. In D67140#1659831 , @NoQ wrote: > In D67140#1659774 , @aaron.ballman > wrote: > > > I don't think it's a requirement (so long as the diagnostics are clear > > about the issue being d

[PATCH] D67140: [analyzer][NFC] Fix inconsistent references to checkers as "checks"

aaron.ballman added a comment. In D67140#1659978 , @Szelethus wrote: > In D67140#1659907 , @NoQ wrote: > > > In D67140#1659872 , @aaron.ballman > > wrote: > > > > > If we're

[PATCH] D64671: [clang-tidy] New check: misc-init-local-variables

aaron.ballman added a comment. Aside from the missing documentation bit, I think this LG. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/InitVariablesCheck.cpp:29 + Options.getLocalOrGlobal("IncludeStyle", "llvm"))), + MathHeader(Options.get("MathHead

[PATCH] D64644: [Sema] Fixes an assertion failure while instantiation a template with an incomplete typo corrected type

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/D64644/new/ https://reviews.llvm.org/D64644 ___ cfe-commits mailing lis

[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/RequireCategoryMethodPrefixesCheck.cpp:57 + } + std::string method_name = method_declaration->getNameAsString(); + auto owning_objc_class_interface = method_declaration->getClassInterface();

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

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/D65917/new/ https://reviews.llvm.org/D65917 ___ cfe-commits mailing lis

[PATCH] D64644: [Sema] Fixes an assertion failure while instantiation a template with an incomplete typo corrected type

aaron.ballman closed this revision. aaron.ballman added a comment. In D64644#1661417 , @Mordante wrote: > Thanks for the review. > Could you commit this patch? I don't have commit access. Happy to do so -- I've commit in r371320. CHANGES SINCE LAST AC

[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access

aaron.ballman added inline comments. Comment at: test/clang-tidy/readability-static-accessed-through-instance.cpp:34 + // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: static member accessed through instance [readability-static-accessed-through-instance] + // CHECK-FIXES: {{^}} C

[PATCH] D36208: [mips] Enable target-specific attributes for MIPS64

aaron.ballman added a comment. In https://reviews.llvm.org/D36208#828881, @atanasyan wrote: > In https://reviews.llvm.org/D36208#828853, @sdardis wrote: > > > Currently there is no support in the backend for the interrupt attribute > > for mips64 / using N32 & N64 abis, it will give a fatal erro

[PATCH] D36208: [mips] Enable target-specific attributes for MIPS64

aaron.ballman added a comment. In https://reviews.llvm.org/D36208#828955, @sdardis wrote: > I think for the interrupt attribute, it should be an error. Currently it's an > implementation detail that it errors out in the backend but in principal it > can be supported (I haven't gotten around to

[PATCH] D36208: [mips] Enable target-specific attributes for MIPS64

aaron.ballman added a comment. In https://reviews.llvm.org/D36208#829006, @sdardis wrote: > @aaron.ballman I missed your first comments when I'd submitted mine. > > In https://reviews.llvm.org/D36208#828957, @aaron.ballman wrote: > > > In https://reviews.llvm.org/D36208#828955, @sdardis wrote: >

[PATCH] D33676: Place implictly declared functions at block scope

aaron.ballman added inline comments. Comment at: lib/Parse/ParseCXXInlineMethods.cpp:521-522 // to be re-used for method bodies as well. - ParseScope FnScope(this, Scope::FnScope|Scope::DeclScope); + ParseScope FnScope(this, Scope::FnScope|Scope::DeclScope| +

[PATCH] D33676: Place implictly declared functions at block scope

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. This looks reasonable to me, once it's been formatted. Let's give @rsmith a few days to comment before committing, though. https://reviews.llvm.org/D33676 ___

[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access

aaron.ballman added inline comments. Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp:23 + memberExpr(hasDeclaration(anyOf(cxxMethodDecl(isStaticStorageClass()), + varDecl(hasStaticStorageDuration(, +

[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker

aaron.ballman added a comment. In https://reviews.llvm.org/D33672#827513, @xazax.hun wrote: > Even though it is not undefined behavior in C, it can still cause surprising > behavior for the users. I think maybe putting it into the optin package > instead of cplusplus is better. What do you thin

[PATCH] D35755: [Solaris] gcc toolchain handling revamp

aaron.ballman added inline comments. Comment at: lib/Driver/ToolChains/Gnu.cpp:1511 + StringRef Suff64 = "/64"; + // Solaris uses platform-specific suffixes instead of /64 + if (TargetTriple.getOS() == llvm::Triple::Solaris) { Add a period at the end of the co

[PATCH] D36208: [mips] Enable `long_call/short_call` attributes on MIPS64

aaron.ballman added inline comments. Comment at: include/clang/Basic/Attr.td:268 def TargetMips : TargetArch<["mips", "mipsel"]>; +def TargetAnyMips : TargetArch<["mips", "mipsel", "mips64", "mips64el"]>; def TargetMSP430 : TargetArch<["msp430"]>; Can you also

[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker

aaron.ballman added a comment. In https://reviews.llvm.org/D33672#830492, @gamesh411 wrote: > As for the the loss of precision problem, in the special case of char the > size of char is known. However does the analysis have the necessary > information in this stage to know the size of an int fo

[PATCH] D35755: [Solaris] gcc toolchain handling revamp

aaron.ballman added a comment. Aside from a coding style nit and the unanswered question that hopefully @tstellar can help answer, this LGTM. I'll wait to accept until we figure out the answer for Linux, however. Comment at: lib/Driver/ToolChains/Solaris.cpp:208 if (GCCIns

[PATCH] D35755: [Solaris] gcc toolchain handling revamp

aaron.ballman added inline comments. Comment at: lib/Driver/ToolChains/Gnu.h:253 +void AddDefaultGCCPrefixes(const llvm::Triple &TargetTriple, + SmallVectorImpl &Prefixes, fedor.sergeev wrote: > aaron.ballman wrote: > > Might a

[PATCH] D36354: [clang-tidy] Implement type-based check for `gsl::owner`

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:29 +void OwningMemoryCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus11) { +return; Can elide the braces. Comm

[PATCH] D36407: [Sema] Extend -Wenum-compare to handle mixed enum comparisons in switch statements

aaron.ballman added a comment. Can you generate the updated patch with more context (https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface)? Comment at: lib/Sema/SemaStmt.cpp:746 +static void checkEnumTypesInSwitchStmt(Sema &S, Expr *Cond, Expr *Ca

[PATCH] D35937: [clang-tidy] Add new readability non-idiomatic static access

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Comment at: clang-tidy/readability/StaticAccessedThroughInstanceCheck.cpp:23 + memberExpr(hasDeclaration(anyOf(cxxMethodDecl(isStaticStorageClass()),

[PATCH] D36411: Restore previous structure ABI for bitfields with 'packed' attribute for PS4 targets

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D36411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman

[PATCH] D36407: [Sema] Extend -Wenum-compare to handle mixed enum comparisons in switch statements

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from a minor naming nit, LGTM! Comment at: lib/Sema/SemaStmt.cpp:605-608 +static QualType GetTypeBeforeIntegralPromotion(const Expr *&expr) { + if (const

[PATCH] D36411: Restore previous structure ABI for bitfields with 'packed' attribute for PS4 targets

aaron.ballman closed this revision. aaron.ballman added a comment. I've commit in r310388 https://reviews.llvm.org/D36411 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36208: [mips] Enable `long_call/short_call` attributes on MIPS64

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, thanks! Repository: rL LLVM https://reviews.llvm.org/D36208 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http:/

[PATCH] D36473: Fix broken getAttributeSpellingListIndex for pragma attributes

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, good catch! https://reviews.llvm.org/D36473 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi

[PATCH] D36526: [Sema] Assign new flag -Wenum-compare-switch to switch-related parts of -Wenum-compare

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D36526 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/

[PATCH] D36354: [clang-tidy] Implement type-based check for `gsl::owner`

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:149 + // Deletion of non-owners, with `delete variable;` + if (DeletedVariable != nullptr) { +assert(DeleteStmt != nullptr && Can `DeletedVariable` ever be n

[PATCH] D36354: [clang-tidy] Implement type-based check for `gsl::owner`

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:188 +diag(OwnerAssignment->getLocStart(), + "assigning neither an owner nor a recognized resource") +<< SourceRange(OwnerAssignment->getLocStart(),

[PATCH] D36354: [clang-tidy] Implement type-based check for `gsl::owner`

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/OwningMemoryCheck.cpp:191 + // Initialization of owners. + else if (OwnerInitialization != nullptr) { +diag(OwnerInitialization->getLocStart(), aaron.ballman wrote: > No else afte

[PATCH] D36672: [clang-tidy] readability-non-const-parameter: fixit on all function declarations

aaron.ballman added inline comments. Comment at: clang-tidy/readability/NonConstParameterCheck.cpp:146 + +if (const auto *Parent = Par->getParentFunctionOrMethod()) { + if (const auto *F = dyn_cast(Parent)) { Please do not use `auto` here, as the type is

[PATCH] D33672: [analyzer] INT50-CPP. Do not cast to an out-of-range enumeration checker

aaron.ballman added inline comments. Comment at: lib/StaticAnalyzer/Checkers/EnumCastOutOfRangeChecker.cpp:19 +// of casting an integer value that is out of range +//===--===// + If this check

[PATCH] D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:29 + unless(anyOf( + isConstexpr(), hasDeclContext(namespaceDecl(isAnonymous())), + hasType(isConstQualified()), -

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.cpp:60 + .bind("wait")); + Finder->addMatcher( + ifStmt(anyOf( Sorry for not noticing this earlier, but I think you can make two

[PATCH] D71174: [clang-tidy] new check: bugprone-signed-char-misuse

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp:97-98 + diag(CastExpression->getBeginLoc(), + "singed char -> integer (%0) conversion; " + "consider to cast to unsigned char first.") + << *Integer

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

aaron.ballman added a comment. In D71846#1796883 , @njames93 wrote: > So I wasn't happy with the vagueness of the else after return check > implementation. Almost finished rewriting the check to properly handle the if > statements with condition variable

[PATCH] D71963: clang-tidy doc: Add the severities description

aaron.ballman added a comment. I think this is a reasonable first-pass at the severity descriptions, but do you think we should add some words that tell the user to use whatever severity is picked by a coding standard if one is being followed? For instance, the CERT rules all come with a severi

[PATCH] D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:29 + unless(anyOf( + isConstexpr(), hasDeclContext(namespaceDecl(isAnonymous())), + hasType(isConstQualified()), -

[PATCH] D70265: [clang-tidy] Add clang tidy check I.2 to cppcoreguidelines

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/AvoidNonConstGlobalVariablesCheck.cpp:29 + unless(anyOf( + isConstexpr(), hasDeclContext(namespaceDecl(isAnonymous())), + hasType(isConstQualified()), -

[PATCH] D71963: clang-tidy doc: Add the severities description

aaron.ballman added a comment. In D71963#1798212 , @sylvestre.ledru wrote: > I do agree that they are subjective and not perfect. > > However, I found the classification extremely useful when you look at the > results on big projects. > I have been usin

[PATCH] D71977: Implement additional traverse() overloads

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/D71977/new/ https://reviews.llvm.org/D71977

[PATCH] D71976: Match code following lambdas when ignoring invisible nodes

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM assuming the behavioral change did not break anything in clang-tools-extra. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D7197

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

aaron.ballman added a comment. In D71846#1798115 , @njames93 wrote: > Sorry I didn't make it obvious with the test cases. The fix will never extend > the lifetime of variables either in the condition or declared in the else > block. It will only apply if

[PATCH] D71982: [docs] Update path to clang-tools-extra

aaron.ballman added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/Contributing.rst:66 -Once you are done, change to the ``llvm/tools/clang/tools/extra`` directory, and +Once you are done, change to the ``llvm/clang-tools-extra`` directory, and let's start! --

[PATCH] D71963: clang-tidy doc: Add the severities description

aaron.ballman added a comment. I may have missed this in prior discussions, and if so, I'm sorry -- but why are we taking CodeChecker as the model for this? I'm not opposed to having some notion of severity for our checks (basically every tool and many coding standards have the same concept), b

[PATCH] D71857: Fixes -Wrange-loop-analysis warnings

aaron.ballman added a comment. It seems like some of the deductions are not spelling out the qualifiers or pointer/references explicitly in some cases, at least from my spot-checking. Can you go back through this patch to make sure we're getting those correct? Comment at: cla

[PATCH] D71980: [clang-tidy] Disable Checks on If constexpr statements in template Instantiations for BugproneBranchClone and ReadabilityBracesAroundStatements

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM with newlines in the test files. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71980/new/ https://reviews.llvm.org/D71980 ___ cfe-commits mailing list cfe-commits@lis

[PATCH] D71499: Add builtins for aligning and checking alignment of pointers and integers

aaron.ballman added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2844 +The builtins ``__builtin_align_up``, ``__builtin_align_down``, return their +first argument aligned up/down to the next multiple of the second argument. +The builtin ``__builtin_is_aligned``

[PATCH] D72018: [attributes] [analyzer] Add an attribute to prevent checkers from modeling a function

aaron.ballman added a comment. > What do you think? Is this reasonable or do you have any alternative ideas? This seems like a very specialized attribute for the static analyzer and I'm not certain how much utility it adds, but that may be because I don't understand the analyzer requirements su

[PATCH] D71963: clang-tidy doc: Add the severities description

aaron.ballman added a comment. In D71963#1800056 , @sylvestre.ledru wrote: > > I may have missed this in prior discussions, and if so, I'm sorry -- but > > why are we taking CodeChecker as the model for this? > > I went ahead and use it because: > > - it

[PATCH] D71963: clang-tidy doc: Add the severities description

aaron.ballman added a comment. In D71963#1800087 , @sylvestre.ledru wrote: > OK, do you want me to prepare a patch to remove the severities? > or to update the values using another list? I think we should remove the severities from the table for now. T

[PATCH] D71499: Add builtins for aligning and checking alignment of pointers and integers

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM with a few nits to be fixed. Comment at: clang/lib/AST/ExprConstant.cpp:8156-8158 + } else if (const Expr *E = Value.Base.dyn_cast()) { +return GetAli

[PATCH] D71963: clang-tidy doc: Add the severities description

aaron.ballman added a comment. In D71963#1800102 , @sylvestre.ledru wrote: > ok, thanks! > I will remove them tomorrow or the next day. > > Do you have any guidance about the next steps to add them back? Yes, sorry about failing to talk about it! I thi

[PATCH] D72049: clang-tidy doc: Remove severities as they don't make consensus

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, thank you for backing this out while we discuss the best way forward. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72049/new

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

aaron.ballman added a comment. In D71846#1800344 , @njames93 wrote: > I'm in two minds about issuing a warning when scope restrictions prevent a > fix. Do you think creating an option to enable or disable emitting warnings > for cases where the scope pre

[PATCH] D71857: [NFC] Fixes -Wrange-loop-analysis warnings

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

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

aaron.ballman added a comment. In D71846#1800401 , @njames93 wrote: > In D71846#1800381 , @aaron.ballman > wrote: > > > In D71846#1800344 , @njames93 > > wrote: > > > > > I

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. I think this generally looks good, thank you for all the hard work on this! I just found some minor nits and testing requests. Assuming no surprises with the tests, LGTM.

[PATCH] D71174: [clang-tidy] new check: bugprone-signed-char-misuse

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/SignedCharMisuseCheck.cpp:97-98 + diag(CastExpression->getBeginLoc(), + "singed char -> integer (%0) conversion; " + "consider to cast to unsigned char first.") + << *Integer

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

aaron.ballman added a comment. In D71846#1800480 , @njames93 wrote: > In D71846#1800411 , @aaron.ballman > wrote: > > > In D71846#1800401 , @njames93 > > wrote: > > > > > I

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

aaron.ballman accepted this revision. aaron.ballman added a comment. The new option LGTM but one of the tests can be updated to have a less complex RUN line. Comment at: clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return.cpp:2-3 +// RUN: %check_clang_tid

[PATCH] D72057: [docs] Update dead anchor in hacking page

aaron.ballman added inline comments. Comment at: clang/www/hacking.html:301 - It is also possible to https://llvm.org/docs/GettingStarted.html#sending-patches-with-git";>use git to contribute to Clang. + It is also possible to https://llvm.org/docs/GettingStarted.html#checko

[PATCH] D71982: [docs] Update path to clang-tools-extra

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. As you mention, we're already inconsistent with how we designate where the repo lives on disk, so I'm fine with landing this as-is and making the root part of the path consistent

[PATCH] D71174: [clang-tidy] new check: bugprone-signed-char-misuse

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/D71174/new/ https://reviews.llvm.org/D71174

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

aaron.ballman accepted this revision. aaron.ballman added a comment. Still LG -- do you need someone to land this on your behalf? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71846/new/ https://reviews.llvm.org/D71846 ___ cfe-commits maili

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

aaron.ballman added inline comments. Comment at: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp:66 + StringRef S = "MyInt target = 0;"; + auto Cat = [&T](StringRef S) { return (T + S).str(); }; + JonasToth wrote: > aaron.ballman wrote: > > I don't thin

[PATCH] D72057: [docs] Update dead anchor in hacking page

aaron.ballman added inline comments. Comment at: clang/www/hacking.html:301 - It is also possible to https://llvm.org/docs/GettingStarted.html#sending-patches-with-git";>use git to contribute to Clang. + It is also possible to https://llvm.org/docs/GettingStarted.html#checko

[PATCH] D72087: [clang-tools-extra] NFC: Fix trivial typos in comments

aaron.ballman closed this revision. aaron.ballman added a comment. Thank you for the patch! I've commit on your behalf in 7ab9acd8f414161b784b61a1633a7c241b82be85 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST AC

[PATCH] D71846: [ASTMatchers] Fix for https://bugs.llvm.org/show_bug.cgi?id=44364

aaron.ballman closed this revision. aaron.ballman added a comment. Thank you for the patch, I've committed on your behalf in ec3d8e61b527c6312f77a4dab095ffc34e954927 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D7184

[PATCH] D72018: [attributes] [analyzer] Add an attribute to prevent checkers from modeling a function

aaron.ballman added a comment. In D72018#1801450 , @xazax.hun wrote: > In D72018#1800018 , @aaron.ballman > wrote: > > > This seems like a very specialized attribute for the static analyzer and > > I'm not certain

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

aaron.ballman added a comment. Still LG with a small nit in one of the FIXME tests. Comment at: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp:1006 +} + +} // namespace test JonasToth wrote: > aaron.ballman wrote: > > JonasToth wrote: > > > aaron.ballm

[PATCH] D72057: [docs] Remove outdated svn/git information from hacking page

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM with a small typo fix. Do you need someone to commit this on your behalf? Comment at: clang/www/hacking.html:279 + To contribute changes to Clang see +

[PATCH] D72018: [attributes] [analyzer] Add an attribute to prevent checkers from modeling a function

aaron.ballman added a comment. In D72018#1802636 , @NoQ wrote: > Would changing the literal in the attribute have the same effect? I.e., > `acquire_handle("Fuchsia_But_Please_Ignore_Me")`. It should, but doesn't currently because we don't have any check

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

aaron.ballman added inline comments. Comment at: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp:1055 + EXPECT_NE(runCheckOnCode(Cat(S), nullptr, "input.m"), +Cat("Object const*target;")); + EXPECT_NE(runCheckOnCode(Cat(S), nullptr, "input.m"),

[PATCH] D72057: [docs] Remove outdated svn/git information from hacking page

aaron.ballman closed this revision. aaron.ballman added a comment. Thank you for the patch! I've commit on your behalf in e5a56f2d50ce1939eba4fddbeb9c8e032db4fc95 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72057/n

[PATCH] D72018: [attributes] [analyzer] Add an attribute to prevent checkers from modeling a function

aaron.ballman added a comment. In D72018#1803144 , @xazax.hun wrote: > In D72018#1803012 , @aaron.ballman > wrote: > > > In D72018#1802636 , @NoQ wrote: > > > > > Would chan

[PATCH] D72140: [clang-tools-extra] NFC: Fix trivial typos in comments

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. This is fantastic, thank you for much for all the typo fixes! This LGTM. Do you need someone to commit on your behalf? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D72167: Add support for __declspec(guard(nocf))

aaron.ballman added a comment. Can you please add some Sema tests that verify the attribute only appertains to functions, accepts the right number of arguments, etc? Also, some tests showing how this attribute works for things like redelcarations and virtual functions would help. e.g., void

[PATCH] D70638: [Diagnostic] add a warning which warns about misleading indentation

aaron.ballman added a comment. In D70638#1803364 , @xbolva00 wrote: > (re-ping; I think this false positive for goto label case is important to be > fixed before 10 release) I agree -- @Tyker, are you planning to work on that fix? Repository: rG LLV

[PATCH] D72140: [clang-tools-extra] NFC: Fix trivial typos in comments

aaron.ballman closed this revision. aaron.ballman added a comment. Thank you for the patch! I've committed on your behalf in b7ecf1c1c373c53183ef6ef66efbe4237ff7b96d . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D72221: Support function attribute patchable_function_entry

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Attr.td:686 +def PatchableFunctionEntry : InheritableAttr { + let Spellings = [GCC<"patchable_function_entry">]; Should this be inheriting from `TargetSpecificAttr` as well given that t

[PATCH] D72233: Add a new AST matcher 'optionally'.

aaron.ballman added a comment. Just to make sure I understand the purpose -- the goal here is to optionally match one or more inner matchers without failing even if none of the inner matchers match anything, and this is a different use case than `anyOf()` because that would fail when none of th

[PATCH] D72289: [analyzer] Update help text to reflect sarif support

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: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72289/new/ https://reviews.llvm.org/D72289

[PATCH] D70689: [analyzer] Fix SARIF column locations

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! I think we've had sufficient time for other reviewers to lodge concerns and we can deal with any other issues post-commit. Do you need me to commit on your behalf? =

<    33   34   35   36   37   38   39   40   41   42   >