[PATCH] D55212: Handle alloc_size attribute on function pointers

aaron.ballman added inline comments. Comment at: test/Misc/pragma-attribute-supported-attributes-list.test:15 +// FIXME: After changing the subject from Function to HasFunctionProto, AllocSize is no longer listed (similar to Format, etc) +// FIXME-NEXT: AllocSize (SubjectMatchRu

[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType

aaron.ballman added inline comments. Comment at: clang/test/Sema/address_space_attribute.cpp:1 +// RUN: %clang_cc1 %s -ast-dump | FileCheck %s + This test should be moved to the AST directory instead of Sema. Comment at: clang/test/Sema/address

[PATCH] D55483: Introduce the callback attribute and emit !callback metadata

aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed. This is missing all of the Sema and SemaCXX tests. Should have tests for member functions, variadic functions, incorrect arguments, incorrect subjects, etc. =

[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType

aaron.ballman added inline comments. Comment at: clang/test/Sema/address_space_attribute.cpp:9 + // CHECK: VarDecl {{.*}} x '__attribute__((address_space(1))) int *' + __attribute__((address_space(1))) int *x; + leonardchan wrote: > aaron.ballman wrote: > > Can

[PATCH] D55534: [AST] Store "UsesADL" information in CallExpr.

aaron.ballman added inline comments. Comment at: include/clang/ASTMatchers/ASTMatchers.h:1260 +/// Matches call expressions which were resolved using ADL. +/// Don't forget to regenerate the documentation and update Registry.cpp to add clang-query support. C

[PATCH] D55534: [AST] Store "UsesADL" information in CallExpr.

aaron.ballman marked an inline comment as done. aaron.ballman added a comment. A few more minor nits. Comment at: docs/LibASTMatchersReference.html:2579-2581 +y(x); Matches +NS::y(x); Doesn't match +y(42); Doesn't match. This is not your bug to fix,

[PATCH] D55534: [AST] Store "UsesADL" information in CallExpr.

aaron.ballman added inline comments. Comment at: docs/LibASTMatchersReference.html:2579-2581 +y(x); Matches +NS::y(x); Doesn't match +y(42); Doesn't match. EricWF wrote: > aaron.ballman wrote: > > This is not your bug to fix, but it seems the document

[PATCH] D55561: Stop stripping comments from AST matcher example code

aaron.ballman created this revision. aaron.ballman added reviewers: JonasToth, klimek. The AST matcher documentation dumping script was being a bit over-zealous about stripping comment markers, which ended up causing comments in example code to stop being comments. This patch fixes that by only

[PATCH] D55482: [clang-tidy] Improve google-objc-function-naming diagnostics 📙

aaron.ballman added inline comments. Comment at: clang-tidy/google/FunctionNamingCheck.cpp:113 + const bool IsGlobal = MatchedDecl->getStorageClass() != SC_Static; diag(MatchedDecl->getLocation(), Drop the top-level `const` qualifier, please.

[PATCH] D55488: Add utility for dumping a label with child nodes

aaron.ballman added inline comments. Comment at: lib/AST/ASTDumper.cpp:89 void dumpDecl(const Decl *D); -void dumpStmt(const Stmt *S); +void dumpStmt(const Stmt *S, const std::string &label = {}); steveire wrote: > aaron.ballman wrote: > > Label >

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

aaron.ballman closed this revision. aaron.ballman added a comment. I've commit in r348889, thank you for the patch! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D51211/new/ https://reviews.llvm.org/D51211 ___ cfe-commits mailing list cfe-co

[PATCH] D55561: Stop stripping comments from AST matcher example code

aaron.ballman closed this revision. aaron.ballman added a comment. Committed in r348891. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55561/new/ https://reviews.llvm.org/D55561 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http:/

[PATCH] D51211: [Sema] Emit -Wformat properly for bitfield promotions.

aaron.ballman reopened this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. I had to revert the commit as the changes caused some test failures. http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/40784/steps/test/logs/stdi

[PATCH] D55447: [Sema] Fix Modified Type in address_space AttributedType

aaron.ballman added inline comments. Comment at: clang/tools/libclang/CXType.cpp:132 + if (!(TU->ParsingOptions & CXTranslationUnit_IncludeAttributedTypes) && + ATT->getAttrKind() != attr::AddressSpace) { return MakeCXType(ATT->getModifiedType(), TU); -

[PATCH] D55410: [clang-tidy] check for flagging using declarations in headers

aaron.ballman added inline comments. Comment at: docs/clang-tidy/checks/abseil-alias-free-headers.rst:13 +The style guide http://google.github.io/styleguide/cppguide.html#Aliases discusses this +issue in more detail Eugene.Zelenko wrote: > Naysh wrote: > > Euge

[PATCH] D55483: Introduce the callback attribute and emit !callback metadata

aaron.ballman added inline comments. Comment at: include/clang/Basic/Attr.td:1204 + VariadicUnsignedArgument<"PayloadIndices">]; + let Subjects = SubjectList<[Function]>; + let Documentation = [Undocumented]; jdoerfert wrote: > aaron.ballman wrote:

[PATCH] D55527: Normalize GlobalDecls when used with CPUDispatch

aaron.ballman added inline comments. Comment at: include/clang/Basic/Attr.td:896 let AdditionalMembers = [{ -IdentifierInfo *getCPUName(unsigned Index) const { - return *(cpus_begin() + Index); +// gets the ordinal of the requested CPU name, or 0 if it isn't in th

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

aaron.ballman added inline comments. Comment at: lib/Sema/SemaDecl.cpp:5971 auto *VD = dyn_cast(&ND); -if (!ND.isExternallyVisible() || (VD && VD->isStaticLocal())) { +NamespaceDecl *NS = NULL; +if (VD) s/NULL/nullptr Also, I think this should b

[PATCH] D58154: Add support for -fpermissive.

aaron.ballman added a comment. I'm not super keen on supporting -fpermissive -- what is the impetus for supporting this? Is it just for GCC compatibility? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58154/new/ https://reviews.llvm.org/D58154 __

[PATCH] D58154: Add support for -fpermissive.

aaron.ballman added a comment. In D58154#1417474 , @rsmith wrote: > In D58154#1417363 , @aaron.ballman > wrote: > > > I'm not super keen on supporting -fpermissive -- what is the impetus for > > supporting this?

[PATCH] D58154: Add support for -fpermissive.

aaron.ballman added a comment. In D58154#1418594 , @jyknight wrote: > The errors disabled by this feature are default-error warnings -- you can > already get the same effect by using -Wno-. Why is it bad to > additionally allow -fpermissive to disable th

[PATCH] D52835: [Diagnostics] Check integer to floating point number implicit conversions

aaron.ballman added a comment. In D52835#1412416 , @xbolva00 wrote: > @aaron.ballman does it make sense to warn for this case only in C/pre-C++11 > mode? Since this case in C++11/14/17 is handled by -Wnarrowing, as we see > from tests. I don't think so

[PATCH] D58757: Add a version of the pass_object_size attribute that works with builtin_dynamic_object_size

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Attr.td:1570 +def PassDynamicObjectSize : InheritableParamAttr { + let Spellings = [Clang<"pass_dynamic_object_size">]; + let Args = [IntArgument<"Type">]; Why use a separate attribute a

[PATCH] D58216: Support attribute used in member funcs of class templates II

aaron.ballman added a comment. In D58216#1414864 , @modocache wrote: > Friendly ping! @aaron.ballman it looks like you accepted D56928 > , could you take a look at this as well? Sorry for the delay -- I was at WG21 meeti

[PATCH] D57855: [analyzer] Reimplement checker options

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from some minor nits, but you should wait for someone more well-versed in the static analyzer to sign off before committing. Comment at: include/cla

[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

aaron.ballman added a comment. Do you have some evidence that the current behavior is causing a lot of false positives in the wild? For ASCII character literals, I can sort of guess at why people might want to do this, but for things like wide character literals, or character literals relying o

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

aaron.ballman added inline comments. Comment at: clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:34 + bool VisitUnqualName(StringRef UnqualName) { +// Check for collisions with function arguments +for (ParmVarDecl *Param : F.parameters()) Missing ful

[PATCH] D58659: [Sema] Fix assertion when `auto` parameter in lambda has an attribute.

aaron.ballman added inline comments. Comment at: clang/lib/Sema/SemaType.cpp:261 +/// necessary. +QualType ReplaceAutoType(QualType TypeWithAuto, QualType Replacement) { + QualType T = sema.ReplaceAutoType(TypeWithAuto, Replacement); I think this wor

[PATCH] D58216: Support attribute used in member funcs of class templates II

aaron.ballman added a comment. In D58216#1420149 , @rafauler wrote: > Both approaches make sense to me. I'll re-land the previous patch in favor of > gcc compatibility, since the semantics of attribute used in member functions > of class templates were f

[PATCH] D58154: Add support for -fpermissive.

aaron.ballman added a comment. In D58154#1420344 , @rsmith wrote: > I don't have particularly strong feelings one way or the other about > accepting `-fpermissive` at all. For GCC compatibility, it seems like a > moderately useful thing to support, but I

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

aaron.ballman added inline comments. Comment at: lib/Sema/SemaDecl.cpp:5970 auto *VD = dyn_cast(&ND); -if (!ND.isExternallyVisible() || (VD && VD->isStaticLocal())) { +const NamespaceDecl *NS = nullptr; +bool isAnonymousNS = false; This can be lo

[PATCH] D59103: [clang-tidy] New checker bugprone-incomplete-comparison-operator

aaron.ballman added a comment. I'm definitely worried about the false positive rate for this check -- from a quick look at the diagnostics from LLVM, every one of them is a false positive (though perhaps I've missed something; it was a very quick look). Requiring users to explicitly use a decla

[PATCH] D59135: Add check for matching HeaderFilter before emitting Diagnostic

aaron.ballman added a comment. Can you add test coverage that demonstrates the fix behaves as expected? Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:454-455 + StringRef FileName(Loc.printToString(Loc.getManager())); + if(getHeaderFilter()->match(Fi

[PATCH] D59135: Add check for matching HeaderFilter before emitting Diagnostic

aaron.ballman added a comment. In D59135#1422781 , @thorsten-klein wrote: > Hello, > Can you please support how to do that? :-) All of the tests live in extra\test\clang-tidy\, so you'd add a file in there. I believe `file-filter.cpp` does stuff with

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

aaron.ballman added inline comments. Comment at: clang/lib/AST/Decl.cpp:2994 +/// +/// \param ConsiderWrapperFunctions If we should consider wrapper functions as +/// their wrapped builtins. This shouldn't be done in general, but its useful in If we should -> If

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

aaron.ballman added a comment. I think we're pretty close! Some of the testing code can be cleaned up, but I could also use some help verifying that we're correctly matching the behavior of GCC as well. Comment at: test/Sema/dllexport-2.cpp:11 +// expected-warning@+3 {{__decl

[PATCH] D59103: [clang-tidy] New checker bugprone-incomplete-comparison-operator

aaron.ballman added a comment. In D59103#1422775 , @kallehuttunen wrote: > Another idea that came to my mind would be to enable this check only for > annotated types. So warning for missing field access would be only given for > types that have for exam

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

aaron.ballman added inline comments. Comment at: clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:180-184 + if (Info.hasMacroDefinition()) { +// The CV qualifiers of the return type are inside macros. +diag(F.getLocation(), Message); +return {}; +

[PATCH] D59223: Objective-C++11: Support static_assert() in @interface/@implementation ivar lists and method declarations

aaron.ballman added inline comments. Comment at: clang/lib/Parse/ParseDecl.cpp:3892-3893 /// +/// Note that a struct declaration refers to a declaration in a struct, +/// not to the declaration of a struct. +/// Seems to be unrelated to this patch? Feel free to

[PATCH] D59223: Objective-C++11: Support static_assert() in @interface/@implementation ivar lists and method declarations

aaron.ballman added inline comments. Comment at: clang/test/Parser/objc-static-assert.mm:1 +// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s + thakis wrote: > aaron.ballman wrote: > > Can you try explicitly specifying C++98 as the underlying langu

[PATCH] D59223: Objective-C++11: Support static_assert() in @interface/@implementation ivar lists and method declarations

aaron.ballman added reviewers: rsmith, aaron.ballman. aaron.ballman added inline comments. Comment at: clang/test/Parser/objc-static-assert.mm:1 +// RUN: %clang_cc1 -fsyntax-only -verify -Wno-objc-root-class %s + erik.pilkington wrote: > thakis wrote: > > aaron.b

[PATCH] D59327: [Sema] Fix a use-after-free of a _Nonnull ParsedAttr

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

[PATCH] D59223: Objective-C++11: Support static_assert() in @interface/@implementation ivar lists and method declarations

aaron.ballman added inline comments. Comment at: clang/test/Parser/objc-static-assert.m:29 +struct S { + @defs(A); +}; Given that static assertions are member declarations of a struct, should this "replay" the static asserts from the interface (including failin

[PATCH] D59223: Objective-C++11: Support static_assert() in @interface/@implementation ivar lists and method declarations

aaron.ballman added inline comments. Comment at: clang/test/Parser/objc-static-assert.m:29 +struct S { + @defs(A); +}; thakis wrote: > aaron.ballman wrote: > > Given that static assertions are member declarations of a struct, should > > this "replay" the static

[PATCH] D59336: [clang-tidy] Disable google-runtime-int in Objective-C++ 🔓

aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed. This is missing test cases that demonstrate the behavior is what we expect it to be for ObjC++ code vs C++ code. Repository: rG LLVM Github Monorepo CHANGES SINCE L

[PATCH] D33841: [clang-tidy] redundant keyword check

aaron.ballman added inline comments. Comment at: clang-tidy/readability/RedundantExternCheck.cpp:27 +void RedundantExternCheck::check(const MatchFinder::MatchResult &Result) { + auto* FD = + Result.Nodes.getNodeAs("redundant_extern"); Formatting is incorrec

[PATCH] D58896: Suppress -Wchar-subscripts if the index is a literal char

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. In D58896#1428816 , @edward-jones wrote: > In D58896#1419964 , @aaron.ballman > wrote: > > > Do

[PATCH] D59336: [clang-tidy] Disable google-runtime-int in Objective-C++ 🔓

aaron.ballman added inline comments. Comment at: clang-tools-extra/test/clang-tidy/google-runtime-int.m:1 +// RUN: clang-tidy -checks=-*,google-runtime-int %s 2>&1 -- -x objective-c | not grep 'warning:\|error:' + We typically use `| count 0` instead of an expli

[PATCH] D59296: [pp-trace] Delete -ignore and add generalized -callbacks

aaron.ballman added a comment. I think the release notes should be updated to mention that a previously-supported option has been removed and that there's a new option available as a replacement; I don't think we have any other docs for pp-trace to worry about updating, do we? ==

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

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45978/new/ https://reviews.llvm.org/D45978 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.o

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from some minor nits. Comment at: clang/lib/Sema/SemaChecking.cpp:338 + case Builtin::BI__builtin___vsnprintf_chk: { +DiagID = diag::warn_memcpy

[PATCH] D59453: [ASTMatchers][OpenMP] Add base ompExecutableDirective() matcher.

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: lib/ASTMatchers/Dynamic/Registry.cpp:508 REGISTER_MATCHER(withInitializer); + REGISTER_MATCHER(ompExecutableDirective);

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

aaron.ballman added inline comments. Comment at: include/clang/AST/ASTTypeTraits.h:81 + /// \{ + /// Return the AST node kind of this ASTNodeKind. These markings are a bit strange, can you explain them to me? Comment at: include/clang/ASTMa

[PATCH] D59463: [ASTMatchers][OpenMP] OpenMP Structured-block-related matchers

aaron.ballman added inline comments. Comment at: include/clang/ASTMatchers/ASTMatchers.h:6421 +/// +/// Prerequisite: the executable directive must not be standalone directive. +/// What happens if this prereq is not met? Does the matcher return false, or does i

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

aaron.ballman added inline comments. Comment at: include/clang/AST/ASTTypeTraits.h:81 + /// \{ + /// Return the AST node kind of this ASTNodeKind. lebedev.ri wrote: > aaron.ballman wrote: > > These markings are a bit strange, can you explain them to me? > It

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

aaron.ballman added inline comments. Comment at: include/clang/AST/ASTTypeTraits.h:81 + /// \{ + /// Return the AST node kind of this ASTNodeKind. lebedev.ri wrote: > aaron.ballman wrote: > > lebedev.ri wrote: > > > aaron.ballman wrote: > > > > These markings

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

aaron.ballman added a comment. I believe this has the incorrect modeling -- in the following code example, the attribute appertains to the substatement of the if statement, not the if statement itself: if (foo) [[likely]] { blah; } This falls out from: http://eel.is/c++draft/stmt.stmt

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Attr.td:1151 +def Likely : StmtAttr { + let Spellings = [CXX11<"", "likely", 201903>, CXX11<"clang", "likely">]; + let Documentation = [LikelyDocs]; I think this should have a C spelling

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

aaron.ballman added inline comments. Comment at: include/clang/ASTMatchers/ASTMatchers.h:6400 +/// \endcode +AST_MATCHER_P(OMPExecutableDirective, hasClause, internal::Matcher, + InnerMatcher) { gribozavr wrote: > Looking at other similar matches, th

[PATCH] D59296: [pp-trace] Delete -ignore and add generalized -callbacks

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from some small nits. Feel free to fix and commit without further review if you'd like. Comment at: docs/ReleaseNotes.rst:141 + +- New `-callbacks`

[PATCH] D57571: [clang-tidy] A new OpenMP module

aaron.ballman added a comment. While I'm okay with this, I would mildly prefer it if landed with at least one check rather than entirely empty (IIRC, that's how we'd added new modules in the past). Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D5757

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

aaron.ballman added a comment. In D59467#1432675 , @Tyker wrote: > if likely/unlikely can be applied to any statement or label what should be > generated when it is applied to a statement that don't have any conditional > branch ? should it be ignored ?

[PATCH] D59455: Thread safety analysis: Add note for unlock kind mismatch

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from a small nit. Comment at: include/clang/Analysis/Analyses/ThreadSafety.h:127 + SourceLocation LocLocked,

[PATCH] D59455: Thread safety analysis: Add note for unlock kind mismatch

aaron.ballman added inline comments. Comment at: include/clang/Analysis/Analyses/ThreadSafety.h:127 + SourceLocation LocLocked, SourceLocation Loc) {} aaronpuchert wrote: > aaron.

[PATCH] D59520: [WebAssembly] Address review comments on r352930

aaron.ballman added a comment. I love functional changes that remove code and add tests -- thank you for these! > Removes errnoneous use of diag::err_alias_is_definition, which turned out to > be ineffective anyway since functions can be defined later in the translation > unit and avoid detecti

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

aaron.ballman closed this revision. aaron.ballman added a comment. > Would you mind committing the changes please? Thanks. Happy to do so! I've committed in r356458. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D45978/new/ https://reviews.llvm.org/D45978

[PATCH] D58757: Add a version of the pass_object_size attribute that works with builtin_dynamic_object_size

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

[PATCH] D59394: [Sema] De-duplicate some availability checking logic

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: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59394/new/ https://reviews.llvm.org/D59394

[PATCH] D59520: [WebAssembly] Address review comments on r352930

aaron.ballman added a comment. In D59520#1435728 , @sunfish wrote: > In D59520#1434854 , @aaron.ballman > wrote: > > > > Removes errnoneous use of diag::err_alias_is_definition, which turned out > > > to be ineffe

[PATCH] D59336: [clang-tidy] Disable google-runtime-int in Objective-C++ 🔓

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

[PATCH] D59523: Thread Safety: also look at ObjC methods

aaron.ballman added a reviewer: delesley. aaron.ballman added a comment. Adding Delesley in case he has input on design. Comment at: lib/Analysis/ThreadSafetyCommon.cpp:280 unsigned I = PV->getFunctionScopeIndex(); - -if (Ctx && Ctx->FunArgs && FD == Ctx->AttrDecl->get

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

aaron.ballman added a comment. I worry a little bit about our -Wtype-limits getting out of sync from GCC's due to making it a synonym for -Wtautological-constant-in-range-compare, but I'm also at a loss for why these two should ever have different functionality. I think I'm okay with this, but

[PATCH] D59628: Add support for __attribute__((objc_class_stub))

aaron.ballman added a comment. You should also add Sema tests that ensure the attribute applies to the expected AST nodes, is diagnosed appropriately when applied to something it shouldn't be applied to, accepts no args, etc. Basically, all of the semantic places we could warn on should have te

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

aaron.ballman added inline comments. Comment at: include/clang/ASTMatchers/ASTMatchers.h:6456 + // Note that we have recieved a *matcher* for the clause, not the + // OpenMPClauseKind. We now need to extract the 'return' type of said matcher, + // and convert it to the OpenMP

[PATCH] D57112: [ASTTypeTraits][ASTMatchers][OpenMP] OMPClause handling

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM aside from a small docs nit (be sure to regenerate the docs after you fix it). Comment at: include/clang/ASTMatchers/ASTMatchers.h:6402 +/// +/// ``ompExecutableDirective(hasClause(anything()))`` matches

[PATCH] D59463: [ASTMatchers][OpenMP] OpenMP Structured-block-related matchers

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM aside from a NFC change Comment at: include/clang/ASTMatchers/ASTMatchers.h:6444-6445 + internal::Matcher, InnerMatcher) { + if (isStandaloneDirective().matches(Node, Finder, Builder)) +

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

aaron.ballman added a reviewer: rsmith. aaron.ballman added a comment. Adding Richard for design strategy discussion. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8164 +def err_must_appear_after_branch : Error< + "%0 can only appear after a selection or iterati

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

aaron.ballman added a comment. In D59467#1439473 , @riccibruno wrote: > Hmm, it seems that an attribute is not allowed by the grammar in the > `expression` or `assignment-expression` of a `conditional-expression`. Was > that intended when `[[likely]]` wa

[PATCH] D59466: [clang-tidy] openmp-exception-escape - a new check

aaron.ballman added inline comments. Comment at: clang-tidy/openmp/ExceptionEscapeCheck.cpp:32 + llvm::StringSet<> IgnoredExceptions; + StringRef(RawIgnoredExceptions).split(IgnoredExceptionsVec, ",", -1, false); + IgnoredExceptions.insert(IgnoredExceptionsVec.begin(), ---

[PATCH] D57113: [clang-tidy] openmp-use-default-none - a new check

aaron.ballman added inline comments. Comment at: clang-tidy/openmp/UseDefaultNoneCheck.cpp:47-48 +diag(Directive->getBeginLoc(), + "OpenMP directive '%0' specifies 'default(%1)' clause. Consider using " + "'default(none)' clause instead.") +<< getOpen

[PATCH] D59466: [clang-tidy] openmp-exception-escape - a new check

aaron.ballman added inline comments. Comment at: clang-tidy/openmp/ExceptionEscapeCheck.cpp:48 +return; + // Similarly, if C++ Exceptions are not enabled, nothing to do. + if (!getLangOpts().CPlusPlus || !getLangOpts().CXXExceptions) lebedev.ri wrote: > aar

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

aaron.ballman added a comment. In D59467#1439585 , @Tyker wrote: > about the semantic issue. > with the standard's latitude on where we can put the attribute, the > attribute can appear any where on the path of execution and must be matched > with the c

[PATCH] D59523: Thread Safety: also look at ObjC methods

aaron.ballman added inline comments. Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:1 +// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class %s + aaronpuchert wrote: > jfb wrote: > > aaronpuchert wrote: > > > Test is fine

[PATCH] D59523: Thread Safety: also look at ObjC methods

aaron.ballman added inline comments. Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:1 +// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class %s + jfb wrote: > aaron.ballman wrote: > > aaronpuchert wrote: > > > jfb wrote: >

[PATCH] D59523: Thread Safety: also look at ObjC methods

aaron.ballman added inline comments. Comment at: test/SemaObjCXX/no-crash-thread-safety-analysis.mm:1 +// RUN: %clang_cc1 -fsyntax-only -verify -Wthread-safety -Wno-objc-root-class %s + jfb wrote: > aaron.ballman wrote: > > jfb wrote: > > > aaron.ballman wrote:

[PATCH] D59466: [clang-tidy] openmp-exception-escape - a new check

aaron.ballman accepted this revision. aaron.ballman added a comment. Aside from a docs nit, LGTM! Comment at: docs/clang-tidy/checks/openmp-exception-escape.rst:13 +structured block. If an exception is not caught in the same structured block +it was thrown in, the behaviour is

[PATCH] D57113: [clang-tidy] openmp-use-default-none - a new check

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM aside from a minor diagnostic wording nit. Comment at: clang-tidy/openmp/UseDefaultNoneCheck.cpp:52 + Clause->getDefaultKind()); +diag(Clause->getBeginLoc(), "e

[PATCH] D59523: Thread Safety: also look at ObjC methods

aaron.ballman added a comment. In D59523#1440238 , @jfb wrote: > In D59523#1440232 , @aaronpuchert > wrote: > > > > It's less about the regressions and more about the kind of regressions > > > we're testing agains

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

aaron.ballman added inline comments. Comment at: clang-tidy/modernize/UseTrailingReturnTypeCheck.cpp:274 + + if (F->getLocation().isInvalid()) +return; bernhardmgruber wrote: > aaron.ballman wrote: > > bernhardmgruber wrote: > > > aaron.ballman wrote: > > >

[PATCH] D58659: [Sema] Fix assertion when `auto` parameter in lambda has an attribute.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Comment at: clang/lib/Sema/SemaType.cpp:261 +/// necessary. +QualType ReplaceAutoType(QualType TypeWithAuto, QualType Replacement) { + QualTy

[PATCH] D58797: [Sema] Add some compile time _FORTIFY_SOURCE diagnostics

aaron.ballman added a comment. In D58797#1439888 , @erik.pilkington wrote: > Ah, I didn't consider that case. Presumably `st` is configured to have > different sizes based on the target? I agree that this is a false-positive, > but it seems like a prett

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

aaron.ballman added a comment. Should this check also try to find this pattern: if (dyn_cast(Bobble)) ... in order to recommend the proper replacement: if (isa(Bobble)) ... I ask because the name `llvm-avoid-cast-in-conditional` sounds like it would also cover this situation and I

[PATCH] D59806: [clang-tidy] Add a check for [super self] in initializers 🔍

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/objc/SuperSelfCheck.cpp:57 +/// \endcode +AST_MATCHER_P(ObjCImplementationDecl, isSubclassOf, + ast_matchers::internal::Matcher, This matcher seems like it would be general

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

aaron.ballman added a comment. In D59802#1443515 , @hintonda wrote: > In D59802#1443451 , @hintonda wrote: > > > In D59802#1443340 , @aaron.ballman > > wrote: > > > > > Shou

[PATCH] D59859: [clang-tidy] FIXIT for implicit bool conversion now obeys upper case suffixes if enforced.

aaron.ballman added a reviewer: alexfh. aaron.ballman added a subscriber: alexfh. aaron.ballman added a comment. > Intended as my first commit to the llvm-project. Welcome! Thank you for working on this! In D59859#1444176 , @lebedev.ri wrote: > Please al

[PATCH] D59467: [clang] Adding the Likely Attribute from C++2a to AST

aaron.ballman added a comment. In D59467#1443173 , @Tyker wrote: > @lebedev.ri where are tests for AST serialization are located ? i didn't find > them. They live in clang\tests\PCH. In D59467#1440608 , @Tyker w

[PATCH] D59628: Add support for __attribute__((objc_class_stub))

aaron.ballman added a comment. In D59628#1446626 , @slavapestov wrote: > I don't know what the etiquette is around here about pinging reviewers for a > re-review, but this CL is ready for another look. Your feedback is much > appreciated! Thanks for le

[PATCH] D59806: [clang-tidy] Add a check for [super self] in initializers 🔍

aaron.ballman added reviewers: jordan_rose, rjmccall. aaron.ballman added a comment. I think this basically LGTM, but I'd appreciate hearing from someone more well-versed in ObjC before landing this. My primary question is: are there situations where `[super self]` is sensible (if so, how should

[PATCH] D59802: [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional

aaron.ballman marked an inline comment as done. aaron.ballman added a comment. In D59802#1445566 , @hintonda wrote: > I looked at the IR generated at `-O2`, and found that while `if (isa(y))` > is a modest win over `if (dyn_cast(y)`, `if (dyn_cast_or_nul

[PATCH] D59806: [clang-tidy] Add a check for [super self] in initializers 🔍

aaron.ballman added a comment. In D59806#1447929 , @jordan_rose wrote: > I don't think there's ever a reason to call `[super self]`, and doing so > through a macro could easily indicate a bug. Thank you for the verification! And agreed about the macro t

<    5   6   7   8   9   10   11   12   13   14   >