[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:96 + // Get the value of the expression to cast. + const auto ValueToCastOptional = + C.getSVal(CE->getSubExpr()).getAs(); NoQ wrote: > aaron.ballman

[PATCH] D53982: Output "rule" information in SARIF

aaron.ballman added inline comments. Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:237-242 +#define GET_CHECKERS +#define CHECKER(FULLNAME, CLASS, CXXFILE, HELPTEXT, GROUPINDEX, HIDDEN) \ + .Case(FULLNAME, HELPTEXT) +#include "clang/StaticAnalyzer/Checkers/Che

[PATCH] D53700: Support _Clang as a scoped attribute identifier

aaron.ballman added reviewers: mclow.lists, EricWF, ldionne. aaron.ballman added a comment. Another option that @rsmith and I discussed today is perhaps using `__clang` or `clang__` as the identifier, but perhaps this will cause more confusion about where to put underscores than `_Clang` would.

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

aaron.ballman added a comment. In https://reviews.llvm.org/D52421#1288910, @lebedev.ri wrote: > And now i'm having concerns. > > struct Base { > void* f; > }; > > struct Inherit : Base { > static void func(void* f) { // <- does 'f' *actually* shadow the 'f' in > the 'Base'? Y

[PATCH] D54169: [clang-tidy] Zircon ->

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.cpp:47 +SrcMgr::CharacteristicKind FileType) { + if (FileName == "fbl/limits.h") { +unsigned End = std::strcspn(SM.getCharacterData(HashLoc), "\n") + 1; D

[PATCH] D54169: [clang-tidy] Zircon ->

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.cpp:47 +SrcMgr::CharacteristicKind FileType) { + if (FileName == "fbl/limits.h") { +unsigned End = std::strcspn(SM.getCharacterData(HashLoc), "\n") + 1; j

[PATCH] D54169: [clang-tidy] Zircon ->

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.cpp:47 +SrcMgr::CharacteristicKind FileType) { + if (FileName == "fbl/limits.h") { +unsigned End = std::strcspn(SM.getCharacterData(HashLoc), "\n") + 1; j

[PATCH] D54169: [clang-tidy] Zircon ->

aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/zircon/FblLimitsCheck.cpp:47 +SrcMgr::CharacteristicKind FileType) { + if (FileName == "fbl/limits.h") { +unsigned End = std::strcspn(SM.getCharacterData(HashLoc), "\n") + 1; j

[PATCH] D54222: [clang-tidy] Add a check to detect returning static locals in public headers

aaron.ballman added inline comments. Comment at: clang-tidy/llvm/ProblematicStaticsCheck.cpp:33 + const auto *VD = Result.Nodes.getNodeAs("var"); + const auto *Return = Result.Nodes.getNodeAs("return"); + diag(Return->getBeginLoc(), "address of static local variable %0 may not

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

aaron.ballman added a comment. Can you add tests for this change? We typically have these in Misc by passing `-ast-print`. Repository: rC Clang https://reviews.llvm.org/D54258 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.

[PATCH] D54246: [clang-tidy] Add the abseil-duration-factory-scale check

aaron.ballman added inline comments. Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:36 +GetScaleForFactory(llvm::StringRef FactoryName) { + static const auto *ScaleMap = + new std::unordered_map( hwright wrote: > Eugene.Zelenko wrote: > > This

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:17 +#include "llvm/ADT/SmallVector.h" +#include "llvm/Support/Mutex.h" + Is this include needed? Comment at: clang-tidy/cppcoreguideline

[PATCH] D54262: [clang-tidy] Add `delete this` bugprone check (PR38741)

aaron.ballman added inline comments. Comment at: clang-tidy/bugprone/DeleteThisCheck.cpp:23 + cxxDeleteExpr( + has(ignoringParenImpCasts(cxxThisExpr( + .bind("DeleteThis"), Will this catch too much? e.g., ``` struct S { int *Foo; ~

[PATCH] D54222: [clang-tidy] Add a check to detect returning static locals in public headers

aaron.ballman added inline comments. Comment at: clang-tidy/llvm/ProblematicStaticsCheck.cpp:33 + const auto *VD = Result.Nodes.getNodeAs("var"); + const auto *Return = Result.Nodes.getNodeAs("return"); + diag(Return->getBeginLoc(), "address of static local variable %0 may not

[PATCH] D54222: [clang-tidy] Add a check to detect returning static locals in public headers

aaron.ballman added inline comments. Comment at: docs/clang-tidy/checks/llvm-problematic-statics.rst:7 + +Looks for functions defined in "public" headers that return the address of a static local. These are not guaranteed to have the addresss across shared libraries and could b

[PATCH] D54281: [clang-tidy] fix PR39583 - ignoring ParenCast for string-literals in pro-bounds-array-to-pointer-decay

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp:65 + unless(isInsideOfRangeBeginEndStmt()), + unless(hasSourceExpression(ignoringParenCasts(stringLiteral() .bind("cast"), ---

[PATCH] D54281: [clang-tidy] fix PR39583 - ignoring ParenCast for string-literals in pro-bounds-array-to-pointer-decay

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/ProBoundsArrayToPointerDecayCheck.cpp:65 + unless(isInsideOfRangeBeginEndStmt()), + unless(hasSourceExpression(ignoringParenCasts(stringLiteral() .bind("cast"), ---

[PATCH] D54307: [clang] overload ignoringParens for Expr

aaron.ballman added inline comments. Comment at: include/clang/ASTMatchers/ASTMatchers.h:814 /// would match the declaration for fp. -AST_MATCHER_P(QualType, ignoringParens, - internal::Matcher, InnerMatcher) { +AST_MATCHER_P_OVERLOAD(QualType, ignoringParens, inter

[PATCH] D54307: [clang] overload ignoringParens for Expr

aaron.ballman added a comment. Also: the patch is missing unit tests. Repository: rC Clang https://reviews.llvm.org/D54307 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54281: [clang-tidy] fix PR39583 - ignoring ParenCast for string-literals in pro-bounds-array-to-pointer-decay

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from a minor commenting nit. Comment at: test/clang-tidy/cppcoreguidelines-pro-bounds-array-to-pointer-decay.cpp:44 +const char *g2() { +return

[PATCH] D53700: Support _Clang as a scoped attribute identifier

aaron.ballman closed this revision. aaron.ballman added a comment. Thanks for the reviews -- I've commit in r346521. https://reviews.llvm.org/D53700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listin

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

aaron.ballman added a comment. In https://reviews.llvm.org/D54258#1292129, @richardmembarth wrote: > I think it's not so easy to provide such tests for CUDA. > CUDA memory space specifiers are implemented via attributes, e.g. `#define > __shared__ __attribute__((shared))`. > As a result of thi

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:108-113 + if (T.isSignedInteger()) +return {llvm::APSInt::getMinValue(Context.getTypeSize(&T), false), +llvm::APSInt::getMaxValue(Context.getTypeSize(&T),

[PATCH] D54307: [clang] overload ignoringParens for Expr

aaron.ballman added inline comments. Comment at: include/clang/ASTMatchers/ASTMatchers.h:814 /// would match the declaration for fp. -AST_MATCHER_P(QualType, ignoringParens, - internal::Matcher, InnerMatcher) { +AST_MATCHER_P_OVERLOAD(QualType, ignoringParens, inter

[PATCH] D54246: [clang-tidy] Add the abseil-duration-factory-scale check

aaron.ballman added inline comments. Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:36 +GetScaleForFactory(llvm::StringRef FactoryName) { + static const auto *ScaleMap = + new std::unordered_map( hokein wrote: > aaron.ballman wrote: > > hwright

[PATCH] D54307: [clang] overload ignoringParens for Expr

aaron.ballman added reviewers: sbenza, klimek. aaron.ballman added a comment. Adding a few other reviewers in case they have ideas on how to use the polymorphic matcher rather than the overload. If they don't have a better idea, then I think the overload approach is fine. Repository: rC Clan

[PATCH] D54307: [clang] overload ignoringParens for Expr

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Comment at: include/clang/ASTMatchers/ASTMatchers.h:814 /// would match the declaration for fp. -AST_MATCHER_P(QualType, ignoringParens, - i

[PATCH] D51575: [clang-tidy/checks] Implement a clang-tidy check to verify Google Objective-C function naming conventions 📜

aaron.ballman added inline comments. Comment at: clang-tidy/google/FunctionNamingCheck.cpp:55 + // to use. + if (Decl->getStorageClass() != SC_Static) { +return FixItHint(); Elide braces. Comment at: clang-tidy/google/FunctionNamingCheck.

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

aaron.ballman added a comment. In https://reviews.llvm.org/D54344#1293468, @erik.pilkington wrote: > Have you tried running creduce on the preprocessed source? We should really > have a real reproducer for this, otherwise its really hard to tell what the > underlying problem is. I'm going to

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

aaron.ballman added a comment. What do you think about code like: #if FOO == 4 #if FOO == 4 #endif #endif #if defined(FOO) #if defined(FOO) #endif #endif #if !defined(FOO) #if !defined(FOO) #endif #endif #if defined(FOO) #if !defined(FOO) #endif #endif

[PATCH] D52984: [analyzer] Checker reviewer's checklist

aaron.ballman added a comment. In https://reviews.llvm.org/D52984#1294223, @xazax.hun wrote: > - Move the checklist up before additional info in the HTML file. > - Fix minor nits. > - Add missing bullet points (thanks @Szelethus for noticing) > > I did not add any coding convention related item

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

aaron.ballman added a comment. In https://reviews.llvm.org/D54349#1294325, @vmiklos wrote: > > What do you think about code like: > > > > #if FOO == 4 > > #if FOO == 4 > > #endif > > #endif > > > > #if defined(FOO) > > #if defined(FOO) > > #endif > > #endif > > > > #if !d

[PATCH] D54344: [Clang][CodeGen][CXX]: Workaround __attribute((no_destroy)) crash/incorrect code generation.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from some small commenting nits. However, I leave it to @erik.pilkington to make the final sign-off. Comment at: lib/CodeGen/CGDeclCXX.cpp:68 + //

[PATCH] D54246: [clang-tidy] Add the abseil-duration-factory-scale check

aaron.ballman added inline comments. Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:39 +static llvm::Optional +GetScaleForFactory(llvm::StringRef FactoryName) { + static const std::unordered_map ScaleMap( GetScaleForFactory -> getScaleForFactory, pe

[PATCH] D51575: [clang-tidy/checks] Implement a clang-tidy check to verify Google Objective-C function naming conventions 📜

aaron.ballman added a comment. Generally LGTM, but I leave it to someone more well-versed in ObjC to sign off that this actually does everything those users would expect. Comment at: docs/clang-tidy/checks/google-objc-function-naming.rst:12 + +All function names should be in u

[PATCH] D54402: Extract method to allow re-use

aaron.ballman added a comment. Generally LGTM but this has no test coverage. What are your plans for how you want to see this proceed? Do you intend to commit everything in one batch once all of the reviews are accepted, or do piecemeal commits? Comment at: lib/ASTMatchers/Dy

[PATCH] D54403: Add new API for returning matching matchers

aaron.ballman added inline comments. Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:604-605 + + std::vector AcceptedTypes; + AcceptedTypes.push_back(StaticType); + `std::vector AcceptedTypes{StaticType};` instead? Comment at: lib/ASTMatcher

[PATCH] D54404: Exclude matchers which can have multiple results

aaron.ballman added inline comments. Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:604 + static std::vector excludedMatchers{ + "allOf", `excludedMatchers` -> `ExcludedMatchers` Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:604

[PATCH] D54405: Record whether a AST Matcher constructs a Node

aaron.ballman added inline comments. Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:70 + void registerIfNodeMatcher(MatcherType, + internal::MatcherDescriptor *matchDescriptor, + StringRef) {} Fix `matchD

[PATCH] D54406: Add matchDynamic convenience functions

aaron.ballman added inline comments. Comment at: include/clang/ASTMatchers/ASTMatchFinder.h:314 +inline SmallVector +matchDynamic(internal::DynTypedMatcher Matcher, const ast_type_traits::DynTypedNode &Node, + ASTContext &Context) { 80-col issue.

[PATCH] D54407: Record the matcher type when storing a binding

aaron.ballman added a reviewer: sbenza. aaron.ballman added a subscriber: sbenza. aaron.ballman added a comment. Adding @sbenza in case he has opinions on this approach. I think it's reasonable, but I also know that changes to the the AST matcher internals sometimes have unintended side effects

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

aaron.ballman added inline comments. Comment at: clang-tidy/readability/ReadabilityTidyModule.cpp:84 +CheckFactories.registerCheck( +"readability-redundant-preprocessor"); CheckFactories.registerCheck( Please keep this list sorted alphabetically.

[PATCH] D54450: Get the correct range of tokens for preprocessor conditions

aaron.ballman created this revision. aaron.ballman added reviewers: rsmith, dblaikie, echristo. Herald added subscribers: jsji, kbarton, nemanjai. While working on https://reviews.llvm.org/D54349, it was noted that the `SourceRange` returned from the preprocessor callbacks was bogus. It was expe

[PATCH] D54402: Extract method to allow re-use

aaron.ballman added a comment. In https://reviews.llvm.org/D54402#1295420, @steveire wrote: > I think this commit is fine without tests. That's not something we do except under specific extenuating circumstances (NFC fixes or changes for which existing coverage suffices), per our developer po

[PATCH] D54403: Add new API for returning matching matchers

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from a minor commenting nit. Comment at: include/clang/ASTMatchers/Dynamic/Registry.h:106 + /// Compute matchers which can be used within a matcher

[PATCH] D54404: Exclude matchers which can have multiple results

aaron.ballman added a reviewer: sbenza. aaron.ballman added a subscriber: sbenza. aaron.ballman added a comment. In https://reviews.llvm.org/D54404#1295426, @steveire wrote: > I acknowledge and share the future-proofing concern. > > We could possibly use something trait-based instead and put the

[PATCH] D54405: Record whether a AST Matcher constructs a Node

aaron.ballman added a comment. Aside from the uses of auto and the lack of tests, this LGTM. Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:77 + internal::MatcherDescriptor *matchDescriptor, StringRef MatcherName) { +auto K = ast_type_traits::ASTNodeKind::getFromNode

[PATCH] D54406: Add matchDynamic convenience functions

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/D54406 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.

[PATCH] D54407: Record the matcher type when storing a binding

aaron.ballman added a comment. In https://reviews.llvm.org/D54407#129, @sbenza wrote: > In https://reviews.llvm.org/D54407#1294934, @aaron.ballman wrote: > > > Adding @sbenza in case he has opinions on this approach. I think it's > > reasonable, but I also know that changes to the the AST ma

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

aaron.ballman added inline comments. Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:645 +getNodeConstructorType(MatcherCtor targetCtor) { + auto const &ctors = RegistryData->nodeConstructors(); + Don't use `auto` here (and if you did. the `const` would go on t

[PATCH] D54407: Record the matcher type when storing a binding

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from a minor formatting nit, LGTM. Comment at: include/clang/ASTMatchers/ASTMatchersInternal.h:154 +bool operator<(const NodeEntry &Other) const { +

[PATCH] D54453: Remove myself as owner of clang-query.

aaron.ballman added a comment. Thank you for all of your hard work on clang-query! I'm happy to pick up the torch as I seem to do a fair amount of the reviews for the tool, but I'm also happy to let Manuel or Alex pick it up if they would prefer. Repository: rL LLVM https://reviews.llvm.or

[PATCH] D54402: Extract method to allow re-use

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D54402#1296273, @steveire wrote: > This is just a NFC change, which is normal to appear without tests. The > consensus on IRC is that this is fine.

[PATCH] D54405: Record whether a AST Matcher constructs a Node

aaron.ballman added inline comments. Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:77 + internal::MatcherDescriptor *matchDescriptor, StringRef MatcherName) { +auto K = ast_type_traits::ASTNodeKind::getFromNodeKind< +typename ast_matchers::internal::VariadicA

[PATCH] D54246: [clang-tidy] Add the abseil-duration-factory-scale check

aaron.ballman added inline comments. Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:57-58 +// One and only one of `IntLit` and `FloatLit` should be provided. +static double GetValue(const IntegerLiteral *IntLit, + const FloatingLiteral *FloatLit

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

aaron.ballman added a comment. In https://reviews.llvm.org/D54258#1295158, @richardmembarth wrote: > There are external tools (e.g. hipacc ) that > generate Clang AST. This AST uses `LangAS` annotations and emits incorrect > memory space specifiers for CUDA when pretty-

[PATCH] D53982: Output "rule" information in SARIF

aaron.ballman added inline comments. Comment at: lib/StaticAnalyzer/Core/SarifDiagnostics.cpp:237-242 +#define GET_CHECKERS +#define CHECKER(FULLNAME, CLASS, CXXFILE, HELPTEXT, GROUPINDEX, HIDDEN) \ + .Case(FULLNAME, HELPTEXT) +#include "clang/StaticAnalyzer/Checkers/Che

[PATCH] D54425: [AArch64] Add aarch64_vector_pcs function attribute to Clang

aaron.ballman added inline comments. Comment at: include/clang/Basic/Attr.td:1792 + let Spellings = [GNU<"aarch64_vector_pcs">, + CXX11<"clang", "aarch64_vector_pcs">, + Keyword<"__aarch64_vector_pcs">, Rather than using GNU a

[PATCH] D54453: Remove myself as owner of clang-query.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. I'll go ahead and add myself as code owner. Peter can remove himself at his leisure. Repository: rL LLVM https://reviews.llvm.org/D54453 __

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

aaron.ballman added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:79-81 + // TODO: Provide an automatic fix if the number is exactly representable in the destination type. + f += 2.0; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: nar

[PATCH] D54404: Exclude matchers which can have multiple results

aaron.ballman added inline comments. Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:606 + // be used with particular arguments. + static std::vector ExcludedMatchers{ + "allOf", sbenza wrote: > I don't think we are allowed to have non-trivial static stor

[PATCH] D51575: [clang-tidy/checks] Implement a clang-tidy check to verify Google Objective-C function naming conventions 📜

aaron.ballman added a comment. In https://reviews.llvm.org/D51575#1297664, @stephanemoore wrote: > Thanks for the review everyone! > > Let me know if there are any further changes that you want me to make or any > further action required on my part to land this 😁 It's good to go in -- do you h

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

aaron.ballman added inline comments. Comment at: lib/Sema/SemaChecking.cpp:113-115 +static void PrettyPrintFloat(const llvm::APFloat &floatValue, + const llvm::fltSemantics &floatSem, + SmallVectorImpl &prettyFloatValue) { -

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

aaron.ballman added a comment. In https://reviews.llvm.org/D52835#1298428, @xbolva00 wrote: > Reverted. > Ok, I will address your comments soon. Thanks! I'm sorry for the delayed reviews -- there were wg21 meetings last week, which several of the reviewers were attending. Repository: rC C

[PATCH] D54450: Get the correct range of tokens for preprocessor conditions

aaron.ballman updated this revision to Diff 174041. aaron.ballman marked 3 inline comments as done. aaron.ballman added a comment. Updating based on review feedback. https://reviews.llvm.org/D54450 Files: include/clang/Lex/Preprocessor.h lib/Lex/PPDirectives.cpp lib/Lex/PPExpressions.cpp

[PATCH] D54450: Get the correct range of tokens for preprocessor conditions

aaron.ballman added a comment. In https://reviews.llvm.org/D54450#1298319, @vmiklos wrote: > Oh, and running the `check-clang-tools` target, this currently results in: > > Failing Tests (3): > Clang Tools :: modularize/ProblemsInconsistent.modularize > Clang Tools :: pp-trace/pp-tra

[PATCH] D54258: [Clang] Fix pretty printing of CUDA address spaces

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D54258#1297687, @richardmembarth wrote: > > In https://reviews.llvm.org/D54258#1297191, @Anastasia wrote: > > > > I agree with the change itself... bu

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

aaron.ballman added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:79-81 + // TODO: Provide an automatic fix if the number is exactly representable in the destination type. + f += 2.0; + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: nar

[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

aaron.ballman added inline comments. Comment at: include/clang/Basic/Attr.td:2096 +def TriviallyRelocatable : InheritableAttr { + let Spellings = [CXX11<"", "trivially_relocatable", 200809>, + CXX11<"clang", "trivially_relocatable">]; erichkean

[PATCH] D54246: [clang-tidy] Add the abseil-duration-factory-scale check

aaron.ballman added inline comments. Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:63 + assert(FloatLit != nullptr && "Neither IntLit nor FloatLit set"); + return FloatLit->getValueAsApproximateDouble(); +} hwright wrote: > aaron.ballman wrote: >

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

aaron.ballman added inline comments. Comment at: clang-tidy/readability/RedundantPreprocessorCheck.cpp:37 +CheckMacroRedundancy(Loc, Condition, IfStack, + "nested redundant if; consider removing it", + "previous if was here", tr

[PATCH] D54547: PTH-- Remove feature entirely-

aaron.ballman added a comment. In https://reviews.llvm.org/D54547#1299105, @chandlerc wrote: > Should likely add release notes about this. > > Also might be worth sending a note to cfe-dev as a heads up and give folks > some time to say "wait wait". +1 to both of these points, but if doesn't c

[PATCH] D54425: [AArch64] Add aarch64_vector_pcs function attribute to Clang

aaron.ballman added a comment. Missing sema tests that demonstrate the attribute can only appertain to functions and types, accepts no arguments, has the expected semantic behavior for typecasts to function pointers, etc. Comment at: include/clang/Basic/Attr.td:1792 + let Sp

[PATCH] D54334: [AVR] Automatically link CRT and libgcc from the system avr-gcc

aaron.ballman added inline comments. Comment at: include/clang/Basic/DiagnosticDriverKinds.td:44-45 +def warn_drv_avr_family_linking_stdlibs_not_implemented: Warning< + "support for linking stdlibs for microcontroller '%0' is not implemented, " + "please file an AVR backend bug

[PATCH] D54334: [AVR] Automatically link CRT and libgcc from the system avr-gcc

aaron.ballman added a comment. I'm not certain if it will be possible to devise test cases for the two diagnostics I pointed out or not, but otherwise, this LGTM as far as the implementation goes. I don't know enough about AVR to sign off on whether the patch logic is correct or not. ===

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

aaron.ballman added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions-long-is-32bits.cpp:4-10 + int i;// i.e. int32_t + long l; // i.e. int32_t + long long ll; // i.e. int64_t + + unsigned int ui;// i.e. uint32_t + un

[PATCH] D54246: [clang-tidy] Add the abseil-duration-factory-scale check

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Comment at: clang-tidy/abseil/DurationFactoryScaleCheck.cpp:73 + case DurationScale::Hours: +if (Multiplier <= 1.0 / 60.0) + return std::make_tu

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

aaron.ballman added inline comments. Comment at: test/clang-tidy/readability-redundant-preprocessor.cpp:53 +// CHECK-NOTES: [[@LINE+1]]:2: warning: nested redundant #if; consider removing it [readability-redundant-preprocessor] +#if FOO == 3 + 1 +// CHECK-NOTES: [[@LINE-3]]:2: n

[PATCH] D54349: [clang-tidy] new check 'readability-redundant-preprocessor'

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. I think this check is in good shape for the initial commit. Additional functionality can be added incrementally. Comment at: test/clang-tidy/readability-redund

[PATCH] D54246: [clang-tidy] Add the abseil-duration-factory-scale check

aaron.ballman added a comment. In https://reviews.llvm.org/D54246#1301940, @hwright wrote: > @aaron.ballman I don't actually have the commit bit, can you commit this, or > are we waiting for further review? No further review required. I'm happy to commit for you. I'll do it later today or tom

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

aaron.ballman added a comment. It looks like you removed a considerable amount of testing coverage; why? Comment at: lib/Sema/SemaChecking.cpp:10920-10921 +if (E->EvaluateAsInt(IntValue, S.Context, Expr::SE_AllowSideEffects)) { + if (S.SourceMgr.isInSystemMacro(CC)) +

[PATCH] D54246: [clang-tidy] Add the abseil-duration-factory-scale check

aaron.ballman closed this revision. aaron.ballman added a comment. I've commit in r347163. Thank you for the patch! https://reviews.llvm.org/D54246 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinf

[PATCH] D54404: Exclude matchers which can have multiple results

aaron.ballman added inline comments. Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:624 + "hasAnyDeclaration", + "hasAnyName", + "hasAnyParameter", steveire wrote: > sbenza wrote: > > I'm not sure what goes in this list. > > `hasAnyName` is here

[PATCH] D54404: Exclude matchers which can have multiple results

aaron.ballman added inline comments. Comment at: lib/ASTMatchers/Dynamic/Registry.cpp:624 + "hasAnyDeclaration", + "hasAnyName", + "hasAnyParameter", steveire wrote: > aaron.ballman wrote: > > steveire wrote: > > > sbenza wrote: > > > > I'm not sur

[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)

aaron.ballman added a comment. Sorry about the delayed review on this -- I had intended to provide comments earlier, but somehow this fell off my radar. Comment at: docs/ReleaseNotes.rst:53 +- ``-Wextra-semi-stmt`` is a new diagnostic, which is, much like + ``-Wextra-semi``

[PATCH] D54704: [clang-tidy] Don't generate incorrect fixes for class constructed from list-initialized arguments

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. > Currently the smart_ptr check (modernize-make-unique) generates the fixes > that cannot compile for cases like below -- because brace list can not be > deduced in make_unique.

[PATCH] D52695: [clang][Parse] Diagnose useless null statements (PR39111)

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from some small nits, LGTM! Comment at: docs/ReleaseNotes.rst:53 +- ``-Wextra-semi-stmt`` is a new diagnostic, which is, much like + ``-Wextra-semi``,

[PATCH] D54450: Get the correct range of tokens for preprocessor conditions

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

[PATCH] D54830: [ASTMatchers] Add hasSideEffect() matcher.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM aside from a minor commenting request. Comment at: include/clang/ASTMatchers/ASTMatchers.h:4121 +/// \brief Matches expressions with potential side effec

[PATCH] D54379: Add Hurd toolchain support to Clang

aaron.ballman added a comment. I don't know enough about hurd to review those portions of it, but I have some general comments on the patch. Comment at: lib/Basic/Targets/OSTargets.h:279 +MacroBuilder &Builder) const override { +// Hurd defines; list ba

[PATCH] D54745: [clang-tidy] Don't generate incorrect fixes for class with deleted copy/move constructor in smart_ptr check.

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Generally LGTM with a few small nits. Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:379 + // + // The fix (std::make_unique) requires to see

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

aaron.ballman added inline comments. Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:42-44 i += 2.0; - // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions] + // CHECK-MESSAG

[PATCH] D54832: [clang-tidy] No fixes for auto new expression in smart check

aaron.ballman added inline comments. Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:255 ASTContext *Ctx) { + // Skip when this is a new-expression with `auto`, e.g. "new auto(1)"." + if (New->getType()->getPointeeType()->getContainedA

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:31 + Options.get("WarnOnFloatingPointNarrowingConversion", 1)), + WarnOnCastingLiterals(Options.get("WarnOnCastingLiterals", 1)) {} + I think

[PATCH] D53655: [ASTImporter] Fix redecl chain of classes and class templates

aaron.ballman added inline comments. Comment at: include/clang/AST/DeclContextInternals.h:128 +DeclsTy &Vec = *getAsVector(); +DeclsTy::iterator I = std::find(Vec.begin(), Vec.end(), D); +return I != Vec.end(); a_sidorin wrote: > `auto I = llvm::find(

[PATCH] D54832: [clang-tidy] No fixes for auto new expression in smart check

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM with a minor typo to correct. Comment at: test/clang-tidy/modernize-make-unique.cpp:285 + // No warninags for `auto` new expression. + PE1.reset(new au

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

aaron.ballman added a comment. I think we're almost there -- I had a few outstanding questions about the config options in the tests and making sure we cover all the cases. Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions-castingliterals-option.cpp:3 +// RU

[PATCH] D53488: [clang-tidy] Improving narrowing conversions

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM! Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53488/new/ https://reviews.llvm.org/D53488 ___ cfe-commits mailing list cfe-commi

[PATCH] D54425: [AArch64] Add aarch64_vector_pcs function attribute to Clang

aaron.ballman added a comment. In D54425#1307838 , @sdesmalen wrote: > Just to double check before committing, @aaron.ballman are you happy with the > tests? The tests LGTM, but there are still some unresolved comments from @rjmccall that should be han

[PATCH] D54902: [AST][Sema] Remove CallExpr::setNumArgs

aaron.ballman added inline comments. Comment at: include/clang/AST/ExprCXX.h:168 public: - CXXMemberCallExpr(ASTContext &C, Expr *fn, ArrayRef args, -QualType t, ExprValueKind VK, SourceLocation RP) - : CallExpr(C, CXXMemberCallExprClass, fn, args, t, V

<    3   4   5   6   7   8   9   10   11   12   >