[PATCH] D56160: [clang-tidy] modernize-use-trailing-return-type check

aaron.ballman added inline comments. Comment at: clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:203 + if (ContainsQualifiers + ContainsSpecifiers + ContainsSomethingElse > 1) +return {}; + bernhardmgruber wrote: > aaron.ballman wrote: > > This should re

[PATCH] D56160: [clang-tidy] modernize-use-trailing-return-type check

aaron.ballman added a comment. In D56160#1496391 , @bernhardmgruber wrote: > @aaron.ballman I do not have commit privileges and I would be very thankful, > if you could commit this patch for me! Thank you! I've commit for you in r360345, thank you for

[PATCH] D56160: [clang-tidy] modernize-use-trailing-return-type check

aaron.ballman reopened this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. In D56160#1496594 , @aaron.ballman wrote: > In D56160#1496391 , @bernhardmgruber > wrote: > > >

[PATCH] D52135: [Clang-Tidy: modernize] Fix for modernize-redundant-void-arg: complains about variable cast to void

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM with the nits corrected (there are still some formatting concerns). Comment at: test/clang-tidy/modernize-redundant-void-arg.cpp:541 +template +void g_3(void){ + // CHECK-MESSAGES: :[[@LINE-1]]:10:

[PATCH] D52157: [ASTMatchers] Let isArrow also support UnresolvedMemberExpr, CXXDependentScopeMemberExpr

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, with a request for the documentation. Comment at: include/clang/ASTMatchers/ASTMatchers.h:4698-4702 /// class Y { /// void x() { this->x(); x(); Y

[PATCH] D51789: [clang] Add the exclude_from_explicit_instantiation attribute

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/AttrDocs.td:2990 +on static and non-static member functions of class templates, static data +members of class templates and member classes of class templates. + }]; An example that demons

[PATCH] D52141: Thread safety analysis: Run more tests with capability attributes [NFC]

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, with a minor formatting nit. Comment at: test/SemaCXX/thread-safety-annotations.h:29 +#define PT_GUARDED_VAR __attribute__((pt_guarded_va

[PATCH] D52179: [clang-tidy] Replace redundant checks with an assert().

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:388 &NamingStyles) { + assert(D->getIdentifier() && !D->getName().empty() && !D->isImplicit() && + "Decl must be an explicit identifier with a na

[PATCH] D52179: [clang-tidy] Replace redundant checks with an assert().

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/clang-tidy/readability/IdentifierNamingCheck.cpp:551-552 if (Decl->isMain() || !Decl->isUserProvided() || -Dec

[PATCH] D52200: Thread safety analysis: Handle ObjCIvarRefExpr in SExprBuilder::translate

aaron.ballman added a reviewer: rjmccall. aaron.ballman added a comment. Adding a reviewer who knows more about ObjC than I do. Comment at: include/clang/Analysis/Analyses/ThreadSafetyCommon.h:400 til::SExpr *translateMemberExpr(const MemberExpr *ME, CallingContext *Ctx); +

[PATCH] D52280: Don't trim non-alphanumeric characters in 'file not found' errors for include directives.

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM! Repository: rC Clang https://reviews.llvm.org/D52280 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50214: Add inherited attributes before parsed attributes.

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

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

aaron.ballman added inline comments. Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:52 +const NamespaceContextVec &Namespaces) { + std::ostringstream Result; + bool First = true; Can this be rewritten with `llvm::for_each()` and a `Twine`

[PATCH] D52281: [clang-tidy] Add modernize check to use std::invoke in generic code

aaron.ballman added inline comments. Comment at: clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp:27-32 + // template + // void f(T func) { + // func(); + // ^~~ + // ::std::invoke(func, 1) + Finder->addMatcher(callExpr(has(declRefExpr())).bind("functor"),

[PATCH] D52339: Support enums with a fixed underlying type in all language modes

aaron.ballman added a reviewer: aaron.ballman. aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. I really like this idea in general, thank you for proposing this patch! Comment at: clang/include/clang/Basic/Fea

[PATCH] D52339: Support enums with a fixed underlying type in all language modes

aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed. Whoops, that acceptance was accidental. Pretending I want changes just to make it clear in Phab. :-) Repository: rC Clang https://reviews.llvm.org/D52339 ___

[PATCH] D52339: Support enums with a fixed underlying type in all language modes

aaron.ballman added a comment. In https://reviews.llvm.org/D52339#1242086, @jfb wrote: > I think we should consider proposing this to the C committee. @aaron.ballman > I can help Erik write the paper, would you be able to present it? Too tight > for the upcoming meeting, but I'm guessing we hav

[PATCH] D52339: Support enums with a fixed underlying type in all language modes

aaron.ballman added a comment. In https://reviews.llvm.org/D52339#1242118, @jfb wrote: > In https://reviews.llvm.org/D52339#1242103, @aaron.ballman wrote: > > > In https://reviews.llvm.org/D52339#1242086, @jfb wrote: > > > > > I think we should consider proposing this to the C committee. > > > @

[PATCH] D52281: [clang-tidy] Add modernize check to use std::invoke in generic code

aaron.ballman added inline comments. Comment at: clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp:27-32 + // template + // void f(T func) { + // func(); + // ^~~ + // ::std::invoke(func, 1) + Finder->addMatcher(callExpr(has(declRefExpr())).bind("functor"),

[PATCH] D52339: Support enums with a fixed underlying type in all language modes

aaron.ballman added a comment. In https://reviews.llvm.org/D52339#1242202, @erik.pilkington wrote: > From the last line in the paper, it seems that C++ compatibility is a goal of > the paper (or at least a consideration). We should probably think about this > if/when the final wording gets acce

[PATCH] D52384: [Sema] Fix redeclaration contexts for enumerators in C

aaron.ballman created this revision. aaron.ballman added reviewers: rsmith, dblaikie. In C, enumerators are not hoisted into, say, a struct decl context when the enumeration is declared inside of a struct. Instead, the enumerators are hoisted into the translation unit decl context. This patch fi

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

aaron.ballman added inline comments. Comment at: clang-tidy/modernize/ConcatNestedNamespacesCheck.cpp:31 +static bool singleNamedNamespaceChild(const NamespaceDecl &ND) { + const NamespaceDecl::decl_range Decls = ND.decls(); + if (std::distance(Decls.begin(), Decls.end()) != 1)

[PATCH] D52400: Improve -Wshadow warnings with enumerators

aaron.ballman created this revision. aaron.ballman added reviewers: rsmith, dblaikie. Currently, we do not check for enumerators that shadow other enumerators as part of -Wshadow, but gcc does provide such a diagnostic for this case. This is intended to catch shadowing issues like: enum E1{e

[PATCH] D52421: [Sema] Diagnose parameter names that shadow inherited field names

aaron.ballman created this revision. aaron.ballman added reviewers: lebedev.ri, rsmith, dblaikie. This patch diagnoses parameter names that shadow the names of inherited fields under -Wshadow-field. It addresses PR34120. Note, unlike GCC, we take into account the accessibility of the field when

[PATCH] D52421: [Sema] Diagnose parameter names that shadow inherited field names

aaron.ballman updated this revision to Diff 166716. aaron.ballman marked an inline comment as done. aaron.ballman added a comment. Updated based on review feedback. https://reviews.llvm.org/D52421 Files: include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/Sema.h lib/Sema/SemaDec

[PATCH] D52421: [Sema] Diagnose parameter names that shadow inherited field names

aaron.ballman added inline comments. Comment at: lib/Sema/SemaDecl.cpp:12380-12382 } + if (LangOpts.CPlusPlus && II) { lebedev.ri wrote: > I think you could move it into the `if()` above? You are correct, I'll hoist it. https://reviews.llvm.org/D52421

[PATCH] D52136: [clang-tidy] Add modernize-concat-nested-namespaces check

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from the local `const` qualification stuff and some minor wordsmithing of the documentation, this LGTM. Comment at: clang-tidy/modernize/ConcatNestedName

[PATCH] D52281: [clang-tidy] Add modernize check to use std::invoke in generic code

aaron.ballman added inline comments. Comment at: clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp:70 + if (MFunctor && MFunctor->isTypeDependent()) { +const auto *Paren = static_cast(MFunctor->getCallee()); +const auto *BinOp = JonasToth wrote: >

[PATCH] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, but you should give @delesley a chance to weigh in before you commit. Repository: rC Clang https://reviews.llvm.org/D52398 ___

[PATCH] D52395: Thread safety analysis: Require exclusive lock for passing by non-const reference

aaron.ballman added a comment. > Unlike checking const qualifiers on member functions, there are probably not > many false positives here: if a function takes a non-const reference, it will > in almost all cases modify the object that we passed it. I'm not certain I agree with the predicate her

[PATCH] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

aaron.ballman added a comment. In https://reviews.llvm.org/D52398#1244074, @aaronpuchert wrote: > No problem. Thanks for reviewing! I'm terribly sorry to be bombarding the two > of you with so many review requests lately, and I hope it'll soon be over. No apologies necessary -- I love and appr

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

aaron.ballman added inline comments. Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:37 + +enum class LengthHandleKind { LHK_Increase, LHK_Decrease }; + Please drop the `LHK_` prefix -- the enum class name is sufficient as a prefix. ===

[PATCH] D52281: [clang-tidy] Add modernize check to use std::invoke in generic code

aaron.ballman added inline comments. Comment at: clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp:70 +const auto *Paren = dyn_cast(MFunctor->getCallee()); +const auto *BinOp = dyn_cast(Paren->getSubExpr()); + How do you know `Paren` won't be null?

[PATCH] D45050: [clang-tidy] New checker for not null-terminated result caused by strlen(), size() or equal length

aaron.ballman added inline comments. Comment at: clang-tidy/bugprone/NotNullTerminatedResultCheck.cpp:1038 + + SmallString<128> NewAddNullTermExprStr; + NewAddNullTermExprStr = "\n"; JonasToth wrote: > whisperity wrote: > > aaron.ballman wrote: > > > This shoul

[PATCH] D52552: [clang-tidy] Flag Classes Inheriting From Structs

aaron.ballman added a comment. This strikes me as likely being a very chatty diagnostic. For instance, it will trigger on harmless code that uses empty base class optimization (in addition to other concerns pointed out by @lebedev.ri). It seems like the real concern here is unintentional inform

[PATCH] D52443: Thread safety analysis: Examine constructor arguments

aaron.ballman added a comment. This generally looks sensible to me. Comment at: lib/Analysis/ThreadSafety.cpp:1970 + // There can be default arguments, so we stop when one iterator is at end(). + for (auto Arg = ArgBegin; Param != Params.end() && Arg != ArgEnd; + ++Para

[PATCH] D51949: [WIP][clang-tidy] initial ideas to isolate variable declarations

aaron.ballman added inline comments. Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:343 + auto Diag = + diag(WholeDecl->getBeginLoc(), "this statement declares %0 variables") + << static_cast( lebedev.ri wrote: > kbobyrev wrote: > > JonasToth

[PATCH] D51949: [WIP][clang-tidy] initial ideas to isolate variable declarations

aaron.ballman added inline comments. Comment at: clang-tidy/readability/IsolateDeclCheck.cpp:343 + auto Diag = + diag(WholeDecl->getBeginLoc(), "this statement declares %0 variables") + << static_cast( JonasToth wrote: > kbobyrev wrote: > > aaron.ballm

[PATCH] D52443: Thread safety analysis: Examine constructor arguments

aaron.ballman added inline comments. Comment at: lib/Analysis/ThreadSafety.cpp:1970 + // There can be default arguments, so we stop when one iterator is at end(). + for (auto Arg = ArgBegin; Param != Params.end() && Arg != ArgEnd; + ++Param, ++Arg) { aaro

[PATCH] D52395: Thread safety analysis: Require exclusive lock for passing by non-const reference

aaron.ballman added a comment. In https://reviews.llvm.org/D52395#1248608, @delesley wrote: > With respect to data, I really think these patches should be tested against > Google's code base, because otherwise you're going to start seeing angry > rollbacks. However, I don't work on the C++ tea

[PATCH] D52384: [Sema] Fix redeclaration contexts for enumerators in C

aaron.ballman added a reviewer: echristo. aaron.ballman added a comment. Ping? https://reviews.llvm.org/D52384 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52400: Improve -Wshadow warnings with enumerators

aaron.ballman added a reviewer: echristo. aaron.ballman added a comment. Ping? https://reviews.llvm.org/D52400 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52384: [Sema] Fix redeclaration contexts for enumerators in C

aaron.ballman updated this revision to Diff 167496. aaron.ballman marked an inline comment as done. aaron.ballman added a comment. Herald added a subscriber: jsji. Updated based on review feedback. https://reviews.llvm.org/D52384 Files: lib/AST/DeclBase.cpp test/Sema/enum.c Index: test/Se

[PATCH] D52384: [Sema] Fix redeclaration contexts for enumerators in C

aaron.ballman added inline comments. Comment at: lib/AST/DeclBase.cpp:1711-1712 + // contexts are always skipped. + while (SkipRecords ? Ctx->isTransparentContext() || Ctx->isRecord() + : Ctx->isTransparentContext()) Ctx = Ctx->getParent();

[PATCH] D52400: Improve -Wshadow warnings with enumerators

aaron.ballman updated this revision to Diff 167507. aaron.ballman marked an inline comment as done. aaron.ballman added a comment. Updating based on review feedback. https://reviews.llvm.org/D52400 Files: lib/Sema/SemaDecl.cpp test/Sema/warn-shadow.c test/SemaCXX/warn-shadow.cpp Index:

[PATCH] D52400: Improve -Wshadow warnings with enumerators

aaron.ballman added inline comments. Comment at: lib/Sema/SemaDecl.cpp:16320 +// Check for other kinds of shadowing not already handled. +CheckShadow(New, PrevDecl, R); + erik.pilkington wrote: > I don't think we should do this for scoped enums in C++, i.

[PATCH] D51949: [clang-tidy] new check 'readability-isolate-declaration'

aaron.ballman added a comment. Can you add some tests for cases like: // C code void (*signal(int sig, void (*func)(int)))(int); // One decl int i = sizeof(struct S { int i;}); // One decl int j = sizeof(struct T { int i;}), k; // Two decls void g(struct U { int i; } s); // One decl v

[PATCH] D52339: Support enums with a fixed underlying type in all language modes

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D52339#1249432, @erik.pilkington wrote: > Ping! This doesn't really affect CUDA or OpenCL more than any change to the > compiler, so I don't think it

[PATCH] D52339: Support enums with a fixed underlying type in all language modes

aaron.ballman added a comment. In https://reviews.llvm.org/D52339#1249531, @rsmith wrote: > In https://reviews.llvm.org/D52339#1249457, @aaron.ballman wrote: > > > In https://reviews.llvm.org/D52339#1249432, @erik.pilkington wrote: > > > > > Ping! This doesn't really affect CUDA or OpenCL more th

[PATCH] D52334: [clang-tidy] Build it even without static analyzer

aaron.ballman added a comment. I think that we need some sort of developer documentation change that explains 1) that this option exists when configuring clang-tidy, and 2) that this impacts the MPI module as well as the static analyzer. Comment at: clang-tidy/CMakeLists.txt:

[PATCH] D52675: [clang] Properly apply attributes on explicit instantiations of static data members

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. This seems reasonable to me, but I'd feel most comfortable if @rsmith also signed off on it. Repository: rC Clang https://reviews.llvm.org/D52675 _

[PATCH] D52334: [clang-tidy] Build it even without static analyzer

aaron.ballman added a comment. In https://reviews.llvm.org/D52334#1249875, @steveire wrote: > @aaron.ballman Happy to add a note to the docs, but your request leaves me > wondering where? > > AFAIK, the fact that `clang-tidy` wasn't built at all when using > `CLANG_ENABLE_STATIC_ANALYZER` is no

[PATCH] D52334: [clang-tidy] Build it even without static analyzer

aaron.ballman added a comment. In https://reviews.llvm.org/D52334#1250166, @steveire wrote: > > I think a reasonable place would be docs/clang-tidy/index.rst in the > > "Getting Involved" area. > > Thanks, that's at least actionable, but not very specific. I've added docs > now. If they don't s

[PATCH] D52334: [clang-tidy] Build it even without static analyzer

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! If you need this merged on your behalf, I can do so tomorrow or Monday, unless someone else gets to it first. Repository: rCTE Clang Tools Extra https://reviews.llvm.or

[PATCH] D52334: [clang-tidy] Build it even without static analyzer

aaron.ballman closed this revision. aaron.ballman added a comment. I've commit as r343415, thank you for the patch! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52334 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://

[PATCH] D52334: [clang-tidy] Build it even without static analyzer

aaron.ballman reopened this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D52334#1250439, @aaron.ballman wrote: > I've commit as r343415, thank you for the patch! I've reverted in r343418 as the commit broke some bots. htt

[PATCH] D52746: [clang-query] Add single-letter 'q' alias for 'quit'

aaron.ballman added a comment. Please update `HelpQuery::run()` to print out the new mnemonic (it's the *only* form of public documentation we have for clang-query currently). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52746 __

[PATCH] D52751: Allow comments with '#' in dynamic AST Matchers

aaron.ballman added inline comments. Comment at: unittests/ASTMatchers/Dynamic/ParserTest.cpp:188-190 + for (size_t i = 0, e = Sema.Errors.size(); i != e; ++i) { +EXPECT_EQ("", Sema.Errors[i]); + } Why are empty errors created? I would have expected this to

[PATCH] D52751: Allow comments with '#' in dynamic AST Matchers

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from some naming and style convention nits, LGTM. Comment at: unittests/ASTMatchers/Dynamic/ParserTest.cpp:151 Sema.parse(" Foo ( Bar ( 17), Baz( \n \"

[PATCH] D52752: [clang-query] Add comment token handling

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from a minor nit. Comment at: clang-query/QueryParser.cpp:42 +End = Begin; +return StringRef(Begin, 0); + } This is just re

[PATCH] D52746: [clang-query] Add single-letter 'q' alias for 'quit'

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52746 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D52398: Thread safety analysis: Unwrap __builtin_expect in getTrylockCallExpr

aaron.ballman added a comment. In https://reviews.llvm.org/D52398#1251894, @aaronpuchert wrote: > @delesley Any objections to this? > > It's certainly useful for our code base, because our `assert`-like macros use > `__builtin_expect`, but I'm not sure if that applies to the general public. I

[PATCH] D52819: Support checking out cte to 'extra' only as backward compatibility

aaron.ballman added a comment. Given the scope of what this changes (such as 3rd party configurations), I think that this requires a broader community discussion than what you'll get from this patch. I would suggest starting an [RFC] thread on cfe-commits and cfe-dev to see what people think of

[PATCH] D52670: [clang-tidy] Add new 'readability-uppercase-literal-suffix' check (CERT DCL16-C, MISRA C:2012, 7.3, MISRA C++:2008, 2-13-4)

aaron.ballman added a comment. Two main points: I don't think this check is covering all of the suffixes (I don't see `q` or `i32` support, for instance), and at least for the CERT rule this is covering, it diagnoses more than it should. The CERT rule is specific to `l` vs `L` but imposes no re

[PATCH] D52281: [clang-tidy] Add modernize check to use std::invoke in generic code

aaron.ballman added inline comments. Comment at: clang-tidy/modernize/ReplaceGenericFunctorCallCheck.cpp:32 + // ::std::invoke(func, 1) + Finder->addMatcher(callExpr(has(declRefExpr())).bind("functor"), this); + I don't think this will work for calls wrapped

[PATCH] D52782: [clang-tidy] Sequence statements with multiple parents correctly (PR39149)

aaron.ballman added inline comments. Comment at: clang-tidy/utils/ExprSequence.cpp:103 for (const Stmt *Parent : getParentStmts(S, Context)) { +// For statements that have multiple parents, make sure we're using the +// parent that lies within the sub-tree under Root.

[PATCH] D52782: [clang-tidy] Sequence statements with multiple parents correctly (PR39149)

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. This seems reasonable to me. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D52782 ___ cfe-commits mailing list cfe

[PATCH] D52727: [clang-tidy] White List Option for performance-unnecessary-value-param, performance-unnecessary-copy-initialization and performance-for-range-copy

aaron.ballman added inline comments. Comment at: clang-tidy/performance/ForRangeCopyCheck.h:43 const bool WarnOnAllAutoCopies; + const std::vector WhiteListTypes; }; Nit: can you name it `AllowedTypes` rather than `WhiteListTypes`? Use similar nomenclature

[PATCH] D52888: Thread safety analysis: Handle conditional expression in getTrylockCallExpr

aaron.ballman added inline comments. Comment at: lib/Analysis/ThreadSafety.cpp:1445 + if (!TCond && FCond) { +Negate = !Negate; +return getTrylockCallExpr(COP->getCond(), C, Negate); Rather than do an assignment here, why not just pass `!Nega

[PATCH] D52870: [NestedNameSpecifier] Add missing stream-specific dump methods

aaron.ballman added inline comments. Comment at: lib/AST/NestedNameSpecifier.cpp:342 void NestedNameSpecifier::dump(const LangOptions &LO) const { + dump(llvm::errs(), LO); `LLVM_DUMP_METHOD` ? Comment at: lib/AST/NestedNameSpecifier.cpp:34

[PATCH] D52888: Thread safety analysis: Handle conditional expression in getTrylockCallExpr

aaron.ballman added inline comments. Comment at: lib/Analysis/ThreadSafety.cpp:1445 + if (!TCond && FCond) { +Negate = !Negate; +return getTrylockCallExpr(COP->getCond(), C, Negate); aaronpuchert wrote: > aaron.ballman wrote: > > Rather than

[PATCH] D52870: [NestedNameSpecifier] Add missing stream-specific dump methods

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

[PATCH] D52888: Thread safety analysis: Handle conditional expression in getTrylockCallExpr

aaron.ballman added a comment. In https://reviews.llvm.org/D52888#1255862, @aaronpuchert wrote: > Additional changes (including some non-tail recursion unfortunately) would > allow the following to work: > > void foo16() { > if (cond ? mu.TryLock() : false) > mu.Unlock(); > } >

[PATCH] D52918: Emit CK_NoOp casts in C mode, not just C++.

aaron.ballman added a comment. Patch is missing tests -- perhaps you could dump the AST and check the casting notation from the output? https://reviews.llvm.org/D52918 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cg

[PATCH] D52771: [clang-tidy] Non-private member variables in classes (MISRA, CppCoreGuidelines, HICPP)

aaron.ballman added a comment. I can't help but notice how badly C.133 and C.9 interact with C.131 and I'm worried we will wind up with clang-tidy checks that leave the user in an impossible situation where they need to make data members private and provide trivial accessors for them. Do you ha

[PATCH] D52771: [clang-tidy] Non-private member variables in classes (MISRA, CppCoreGuidelines, HICPP)

aaron.ballman added a comment. In https://reviews.llvm.org/D52771#1256511, @JonasToth wrote: > In https://reviews.llvm.org/D52771#1256432, @aaron.ballman wrote: > > > I can't help but notice how badly C.133 and C.9 interact with C.131 and I'm > > worried we will wind up with clang-tidy checks th

[PATCH] D52918: Emit CK_NoOp casts in C mode, not just C++.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, thank you for adding the test and the TODO! https://reviews.llvm.org/D52918 ___ cfe-commits mailing list cfe-commits@lists.ll

[PATCH] D52888: Thread safety analysis: Handle conditional expression in getTrylockCallExpr

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D52888#1256625, @aaronpuchert wrote: > In https://reviews.llvm.org/D52888#1256395, @aaron.ballman wrote: > > > In https://reviews.llvm.org/D52888#12558

[PATCH] D57472: [AST] Extract ASTDumpTraverser class from ASTDumper

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Herald added a project: clang. LGTM with a renaming request. Comment at: include/clang/AST/ASTDumpTraverser.h:83 + + NodeVisitorType &getNodeVisitor() { return

[PATCH] D57540: [C++17] Don't crash while diagnosing different access specifier of a deduction guide.

aaron.ballman added a reviewer: aaron.ballman. aaron.ballman added inline comments. Herald added a project: clang. Comment at: test/SemaCXX/cxx1z-class-template-argument-deduction.cpp:360-361 + struct Type { +Typo(); // expected-error{{deduction guide must be declared in the

[PATCH] D56555: Add Attribute to define nonlazy objc classes

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

[PATCH] D57472: [AST] Extract ASTDumpTraverser class from ASTDumper

aaron.ballman added inline comments. Comment at: include/clang/AST/ASTDumpTraverser.h:1 +//===--- ASTDumpTraverser.h - Traversal of AST nodes --===// +// steveire wrote: > I think I'll rename this to `ASTNodeTraverser`. Any objections? Nope,

[PATCH] D57649: [ASTDump] Add a flag indicating whether a CXXThisExpr is implicit

aaron.ballman added a comment. In D57649#1382470 , @riccibruno wrote: > Though it is a subjective distinction and in the end if someone need a flag > it is going to be added I guess. That's the crux of how we've decided what information to expose in the

[PATCH] D57540: [C++17] Don't crash while diagnosing different access specifier of a deduction guide.

aaron.ballman added inline comments. Comment at: test/SemaCXX/cxx1z-class-template-argument-deduction.cpp:360-361 + struct Type { +Typo(); // expected-error{{deduction guide must be declared in the same scope}} +// expected-error@-1{{deduction guide declaration without

[PATCH] D57207: [clang-tidy] Make google-objc-function-naming ignore implicit functions 🙈

aaron.ballman added inline comments. Comment at: test/clang-tidy/google-objc-function-naming.m:3 +#import + stephanemoore wrote: > It turns out importing is problematic and breaks the build (though > everything built successfully for me locally 🤔). I believe

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added a comment. In D45978#1382292 , @zahiraam wrote: > In D45978#1379901 , @rnk wrote: > > > I'm still not sure this is the best place to make this change, but the > > functionality is important. The

[PATCH] D57659: [Sema] SequenceChecker: Add some comments + related small NFCs in preparation of the following patches

aaron.ballman added inline comments. Comment at: lib/Sema/SemaChecking.cpp:11748 +/// The expressions corresponding to each usage kind. +Expr *UsageExprs[UK_Count]{}; + You can drop the `{}` here and below. Comment at: lib/Sema/SemaChec

[PATCH] D57674: [clang-tidy] Add options to bugprone-argument-comment to add missing argument comments to literals

aaron.ballman added a comment. Can you drop the file mode changes that are in this review? Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:292-293 +// If the argument comments are missing for literals add them. +if (Comments.empty()) { + if (((isa(Args[I])

[PATCH] D57086: Ignore trailing NullStmts in StmtExprs for GCC compatibility

aaron.ballman added inline comments. Comment at: clang/include/clang/AST/Stmt.h:1259-1260 + // This gets the index of the last Stmt before the trailing NullStmts. If + // this compound expression contains nothing but NullStmts, then we return + // the index of the last one. If

[PATCH] D56850: [ASTMatchers][NFC] Add tests for assorted `CXXMemberCallExpr` matchers.

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

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

aaron.ballman added inline comments. Comment at: lib/AST/Expr.cpp:2562 +return ICE->getSubExpr(); + + else if (auto *FE = dyn_cast_or_null(E)) riccibruno wrote: > It is something that is actually possible to audit. I did look at where each > of the skipped

[PATCH] D57674: [clang-tidy] Add options to bugprone-argument-comment to add missing argument comments to literals

aaron.ballman added inline comments. Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:228-236 +static bool isStringLiteral(const Expr *Arg) { + const auto *Cast = dyn_cast(Arg); + return Cast ? isa(Cast->getSubExpr()) : false; +} + +static bool isNullPtrLiteral(const Ex

[PATCH] D57787: [clang-tidy] modernize-avoid-c-arrays: avoid main function (PR40604)

aaron.ballman added inline comments. Comment at: clang-tidy/modernize/AvoidCArraysCheck.cpp:35 + const clang::DeclContext *DC = Node.getDeclContext(); + const clang::FunctionDecl *FD = llvm::dyn_cast(DC); + if (!FD) JonasToth wrote: > lebedev.ri wrote: > > leb

[PATCH] D57674: [clang-tidy] Add options to bugprone-argument-comment to add missing argument comments to literals

aaron.ballman added inline comments. Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:228-236 +static bool isStringLiteral(const Expr *Arg) { + const auto *Cast = dyn_cast(Arg); + return Cast ? isa(Cast->getSubExpr()) : false; +} + +static bool isNullPtrLiteral(const Ex

[PATCH] D45978: dllexport const variables must have external linkage.

aaron.ballman added a comment. Can you add tests for C mode as well, as it seems the behavior differs there. Given: __declspec(dllexport) int const x = 3; __declspec(dllexport) const int y; extern int const z = 4; int main() { int a = x + y + z; return a; } I get: C:\Us

[PATCH] D57787: [clang-tidy] modernize-avoid-c-arrays: avoid main function (PR40604)

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from a nit. Comment at: clang-tidy/modernize/AvoidCArraysCheck.cpp:36-38 + if (!FD) // If not a FunctionDecl, then certainly not main entry function

[PATCH] D57267: [AST] Factor out the logic of the various Expr::Ignore*

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! You should give @rsmith a few days in case he wants to express an opinion though. Comment at: lib/AST/Expr.cpp:2643-2645 + } + + else if (auto *GSE = d

[PATCH] D57322: [ASTImporter] Refactor unittests to be able to parameterize them in a more flexible way

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM, though I am by no means a gtest expert. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57322/new/ https://reviews.llvm.org/D57322 ___ cfe-comm

[PATCH] D57674: [clang-tidy] Add options to bugprone-argument-comment to add missing argument comments to literals

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from a testing nit. Comment at: clang-tidy/bugprone/ArgumentCommentCheck.cpp:231 +bool ArgumentCommentCheck::shouldAddComment(const Expr *Arg) const

[PATCH] D57918: Add an attribute that causes clang to emit fortified calls to C stdlib functions

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Attr.td:1572 + let Spellings = [Clang<"fortify_stdlib">]; + let Args = [IntArgument<"Type">, IntArgument<"Flag">]; + let Subjects = SubjectList<[Function]>; Nothing in the docs describe

<    7   8   9   10   11   12   13   14   15   16   >