[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier

aaron.ballman added a reviewer: rsmith. aaron.ballman added a comment. Adding Richard to the review for some wider perspective than just mine on the overall design. Comment at: lib/Sema/AnalysisBasedWarnings.cpp:296 + + if (ThrowType->isReferenceType()) +ThrowType = Throw

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

aaron.ballman added inline comments. Comment at: clang-tidy/readability/RedundantKeywordCheck.cpp:22 +template +static bool startsWith(const T &Node, StringRef Value) { + Token Result; Why do you need to do a textual search that the first token in the declarati

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

aaron.ballman requested changes to this revision. aaron.ballman added inline comments. This revision now requires changes to proceed. Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:41 + forEachConstructorInitializer( + cxxCtorInitializer(

[PATCH] D34091: Support for querying the exception specification type through libclang

aaron.ballman added inline comments. Comment at: include/clang-c/Index.h:185 + */ + CXCursor_ExceptionSpecificationKind_None, ///< no exception specification + You can drop the trailing comment. Comment at: include/clang-c/Index.h:208 + /**

[PATCH] D33904: Add a __has_attribute_enhancement macro to clang

aaron.ballman added a comment. In https://reviews.llvm.org/D33904#778165, @george.burgess.iv wrote: > > Why not just use __has_feature(overloadable_unmarked) or similar? > > My impression was that `__has_feature` was was for larger features than > tweaks to attributes. If this would be an approp

[PATCH] D34179: [Frontend] PR32318 Handle -ast-dump-all properly.

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

[PATCH] D34179: [Frontend] PR32318 Handle -ast-dump-all properly.

aaron.ballman closed this revision. aaron.ballman added a comment. Committed in r305432. You should consider reaching out to Chris Lattner for svn access! https://reviews.llvm.org/D34179 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http:

[PATCH] D34091: Support for querying the exception specification type through libclang

aaron.ballman added inline comments. Comment at: include/clang-c/Index.h:213 + /** + * \brief The exception specification has not yet been evaluated + */ This comment is now missing the full-stop at the end of the sentence (you dropped one too many periods)

[PATCH] D33722: [clang-tidy] Add checker for undelegated copy of base classes

aaron.ballman added inline comments. Comment at: clang-tidy/misc/UndelegatedCopyOfBaseClassesCheck.cpp:92 + // Loop until the beginning of the initialization list. + while (!Tok.is(tok::r_paren)) +Lex.LexFromRawLexer(Tok); szdominik wrote: > aaron.ballman w

[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier

aaron.ballman added inline comments. Comment at: lib/Sema/AnalysisBasedWarnings.cpp:304 + ->getUnqualifiedDesugaredType(); + if (CaughtType == nullptr || CaughtType == ThrowType) +return true; aaron.ballman wrote: > The check for a null

[PATCH] D33563: Track whether a unary operation can overflow

aaron.ballman marked an inline comment as done. aaron.ballman added a comment. Ping https://reviews.llvm.org/D33563 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D32332: Add support for transparent overloadable functions in clang

aaron.ballman added a comment. This is generally looking good to me, with a few small nits. @rsmith, do you have thought on this? Comment at: lib/Sema/SemaDecl.cpp:2875-2876 /// Returns true if there was an error, false otherwise. -bool Sema::MergeFunctionDecl(FunctionDecl *N

[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier

aaron.ballman added a comment. A few more nits, but this feels like it's getting close to ready (at least, to me). Comment at: test/SemaCXX/warn-throw-out-noexcept-func.cpp:1 +// RUN: %clang_cc1 %s -fdelayed-template-parsing -fcxx-exceptions -fexceptions -fsyntax-only -Wexce

[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier

aaron.ballman added inline comments. Comment at: test/SemaCXX/warn-throw-out-noexcept-func.cpp:1 +// RUN: %clang_cc1 %s -fdelayed-template-parsing -fcxx-exceptions -fexceptions -fsyntax-only -Wexceptions -verify -std=c++11 +struct A { rnk wrote: > aaron.ballman

[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier

aaron.ballman added inline comments. Comment at: test/SemaCXX/warn-throw-out-noexcept-func.cpp:27 +} + +struct N : A { jyu2 wrote: > aaron.ballman wrote: > > Can you add a test case like: > > ``` > > struct Throws { > > ~Throws() noexcept(false); > > }; > > >

[PATCH] D34449: [clang-tidy] Enable constexpr definitions in headers.

aaron.ballman added inline comments. Comment at: test/clang-tidy/misc-definitions-in-headers.hpp:180 +class CE { + constexpr static int i = 5; // OK: constexpr definition. +}; This is not as safe as you might think. As-is, this is fine, however, if the class is

[PATCH] D33333: Emit warning when throw exception in destruct or dealloc functions which has a (possible implicit) noexcept specifier

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from a few small nits with the test cases, this LGTM! You should hold off on committing for a day or two in case Richard or others have comments on the patch. ==

[PATCH] D34091: Support for querying the exception specification type through libclang

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

[PATCH] D33470: [clang-tidy] Add misc-default-numerics

aaron.ballman added a comment. In https://reviews.llvm.org/D33470#789791, @Prazek wrote: > In https://reviews.llvm.org/D33470#764846, @aaron.ballman wrote: > > > Once you fix the typo in the check, can you run it over some large C++ code > > bases to see if it finds any results? > > > I tried it

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

aaron.ballman added a comment. In https://reviews.llvm.org/D40671#954906, @xgsa wrote: > In https://reviews.llvm.org/D40671#954661, @alexfh wrote: > > > In https://reviews.llvm.org/D40671#953888, @aaron.ballman wrote: > > > > > FWIW, I think we should do something about unknown check names in NOL

[PATCH] D40671: [clang-tidy] Support specific checks for NOLINT directive

aaron.ballman closed this revision. aaron.ballman added a comment. Committed in r320713. https://reviews.llvm.org/D40671 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40813: [clang-tidy] Adding Fuchsia checker for virtual inheritance

aaron.ballman added inline comments. Comment at: test/clang-tidy/fuchsia-virtual-inheritance.cpp:34-36 + // CHECK-MESSAGES: [[@LINE-1]]:28: warning: constructing a class that inherits a virtual base class is disallowed [fuchsia-virtual-inheritance] + // CHECK-NEXT: D(int valu

[PATCH] D40819: Implement Attribute Target MultiVersioning (Improved edition!)

aaron.ballman added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9333-9335 + "multiversion function would have identical mangling to a previous " + "definition. Duplicate declarations must have identical target attribute " + "values">; -

[PATCH] D41224: [ThreadSafetyAnalysis] Fix isCapabilityExpr

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. This LGTM, but you should wait to commit for a few days in case @delesley has any concerns. Repository: rC Clang https://reviews.llvm.org/D41224 __

[PATCH] D40625: Harmonizing attribute GNU/C++ spellings

aaron.ballman closed this revision. aaron.ballman added a comment. Committed (with additional comments on deviating attributes) in r320752. https://reviews.llvm.org/D40625 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org

[PATCH] D40813: [clang-tidy] Adding Fuchsia checker for virtual inheritance

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from some documentation nits, LGTM! Comment at: docs/clang-tidy/checks/fuchsia-virtual-inheritance.rst:6 + +Warns if classes are defined or created with v

[PATCH] D41303: Add support for ObjectFormat to TargetSpecificAttr

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from some minor nits, LGTM. Thanks! Comment at: include/clang/Basic/Attr.td:281 list CXXABIs; + // Specifies Object Formats for which the target appl

[PATCH] D41363: [clang-tidy] Adding Fuchsia checker for overloaded operators

aaron.ballman added inline comments. Comment at: clang-tidy/fuchsia/OverloadedOperatorCheck.cpp:18 + +AST_MATCHER(CXXMethodDecl, hasOverloadedOperator) { + if (Node.isCopyAssignmentOperator() || Node.isMoveAssignmentOperator()) JonasToth wrote: > I think `isOver

[PATCH] D41455: [ASTMatchers] Add isNoReturn() match narrower for FunctionDeclarations

aaron.ballman added a comment. You regenerate the HTML by running `clang/docs/tools/dump_ast_matchers.py`. Comment at: unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp:1775 + EXPECT_TRUE(notMatches("void func();", functionDecl(isNoReturn(; + EXPECT_TRUE(notMatches("void

[PATCH] D41456: [clang-tidy] readability-else-after-return: also diagnose noreturn function calls.

aaron.ballman added inline comments. Comment at: clang-tidy/readability/ElseAfterReturnCheck.cpp:43 + for (const auto *BindingName : + {"return", "continue", "break", "throw", "noreturn call"}) if (Result.Nodes.getNodeAs(BindingName)) You've spotted t

[PATCH] D40448: Add a printing policy for AST dumping

aaron.ballman added a comment. P-p-p-power ping! :-) https://reviews.llvm.org/D40448 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D18768: Refactoring attribute subject diagnostics

aaron.ballman abandoned this revision. aaron.ballman added a comment. Closing this via "abandon" so that Richard doesn't have to accept it in order for me to close it. It's already been committed. https://reviews.llvm.org/D18768 ___ cfe-commits mai

[PATCH] D40448: Add a printing policy for AST dumping

aaron.ballman closed this revision. aaron.ballman marked an inline comment as done. aaron.ballman added a comment. Thanks, Hal! Committed in r321223. Comment at: lib/AST/ASTDumper.cpp:218 +: ASTDumper(OS, Traits, SM, ShowColors, LangOptions()) {} +//ASTDumper(raw_os

[PATCH] D41363: [clang-tidy] Adding Fuchsia checker for overloaded operators

aaron.ballman added a comment. Are the Fuchsia library headers intended to also comply with this rule? I notice there's mention of a unique_ptr class, and I can't imagine that working without overloading more operators than just assignment. Perhaps this check should not be triggered for system

[PATCH] D41363: [clang-tidy] Adding Fuchsia checker for overloaded operators

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D41363#962727, @juliehockett wrote: > > Are the Fuchsia library headers intended to also comply with this rule? I > > notice there's mention of a uniq

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

aaron.ballman added inline comments. Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:21-22 #include "ClangTidyOptions.h" +#include "ClangTidy.h" +#include "ClangTidyModule.h" #include "clang/AST/ASTDiagnostic.h" Please keep the includes in sorted order.

[PATCH] D41546: [clang-tidy] Adding Fuchsia checker for statically constructed objects

aaron.ballman added inline comments. Comment at: clang-tidy/fuchsia/StaticallyConstructedObjectsCheck.cpp:23 + // Match statically stored objects... + hasStaticStorageDuration(), + // ... which have C++ constructors... --

[PATCH] D41553: Support parsing double square-bracket attributes in ObjC

aaron.ballman created this revision. aaron.ballman added a reviewer: rsmith. One plausible use for the new double square-bracket attribute support in C are attributes appertaining to Objective-C constructs. This patch adds parsing support for such attributes and exposes a handful of ObjC attribu

[PATCH] D41553: Support parsing double square-bracket attributes in ObjC

aaron.ballman added a comment. This patch depends on https://reviews.llvm.org/D41317 for the modifications to the Clang<> attribute spelling. https://reviews.llvm.org/D41553 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.

[PATCH] D41553: Support parsing double square-bracket attributes in ObjC

aaron.ballman added reviewers: rjmccall, arphaman, ahatanak. aaron.ballman added a comment. In https://reviews.llvm.org/D41553#963536, @rsmith wrote: > This appears to add attributes on some Obj-C constructs as an on-by-default > feature in Obj-C++11 onwards; it needs review by folks more heavil

[PATCH] D41512: [Sema] -Wtautological-constant-compare is too good. Cripple it.

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

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

aaron.ballman added inline comments. Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:161 + + const SmallVector& getCheckNames() const { +assert(isNolintFound()); aaron.ballman wrote: > The `&` should bind to the right. However, instead of returning th

[PATCH] D41455: [ASTMatchers] Add isNoReturn() match narrower for FunctionDeclarations

aaron.ballman accepted this revision. aaron.ballman added a subscriber: dblaikie. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from a documentation nit, this LGTM. However, you should wait to commit until after you can safely regenerate the AST matcher doc

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

aaron.ballman added inline comments. Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:424 +// This method is not const, because it fills cache on first demand. +// It is possible to fill cache in constructor or make cache volatile +// and make this method const, but they se

[PATCH] D41646: [Sema] Improve diagnostics for const- and ref-qualified member functions

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Btw, if you get the LG on a patch but the patch breaks bots on commit, you generally don't need to do another round of review unless you need to highlight significant chan

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

aaron.ballman added a comment. I think this check is going to be extraordinarily chatty. For instance, macros are often used to hide compiler details in signatures, such as use of attributes. This construct cannot be replaced with anything else because the macro isn't defining an object or valu

[PATCH] D41648: [clang-tidy] implement cppcoreguidelines macro rules

aaron.ballman added a comment. In https://reviews.llvm.org/D41648#965437, @JonasToth wrote: > In https://reviews.llvm.org/D41648#965432, @aaron.ballman wrote: > > > I think this check is going to be extraordinarily chatty. For instance, > > macros are often used to hide compiler details in signa

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

aaron.ballman added inline comments. Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:62-63 +void UnusedReturnValueCheck::check(const MatchFinder::MatchResult &Result) { + if (const auto Matched = Result.Nodes.getNodeAs("match")) { +diag(Matched->getLocStart(), "un

[PATCH] D40787: [clang-tidy] Replace the usage of std::uncaught_exception with std::uncaught_exceptions

aaron.ballman added inline comments. Comment at: clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp:24 + + const char *MatchText = "::std::uncaught_exception"; + You might as well make this a `std::string` rather than `const char *` because the `hasName()` mat

[PATCH] D40787: [clang-tidy] Replace the usage of std::uncaught_exception with std::uncaught_exceptions

aaron.ballman added inline comments. Comment at: clang-tidy/modernize/UseUncaughtExceptionsCheck.cpp:66-67 +} +Diag << FixItHint::CreateInsertion(BeginLoc.getLocWithOffset(TextLength), + "s"); + } koldaniel wrote: >

[PATCH] D41317: Infrastructure for adding C attributes

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

[PATCH] D15935: Improve diagnostics for literal conversion to Boolean

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

[PATCH] D41553: Support parsing double square-bracket attributes in ObjC

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

[PATCH] D33563: Track whether a unary operation can overflow

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

[PATCH] D41512: [Sema] -Wtautological-constant-compare is too good. Cripple it.

aaron.ballman added inline comments. Comment at: include/clang/Basic/DiagnosticGroups.td:440 def TautologicalUnsignedEnumZeroCompare : DiagGroup<"tautological-unsigned-enum-zero-compare">; +def TautologicalRangeCompare : DiagGroup<"tautological-constant-range-compare">; def T

[PATCH] D41512: [Sema] -Wtautological-constant-compare is too good. Cripple it.

aaron.ballman added inline comments. Comment at: include/clang/Basic/DiagnosticGroups.td:440 def TautologicalUnsignedEnumZeroCompare : DiagGroup<"tautological-unsigned-enum-zero-compare">; +def TautologicalRangeCompare : DiagGroup<"tautological-constant-range-compare">; def T

[PATCH] D41553: Support parsing double square-bracket attributes in ObjC

aaron.ballman added inline comments. Comment at: include/clang/Basic/Attr.td:239 + bit IncludeC = includeC; +} rjmccall wrote: > I have no objection to allowing ObjC attributes to be spelled in > [[clang::objc_whatever]] style. We can debate giving them a mo

[PATCH] D41708: [clang-tidy] Update fuchsia-overloaded-operator to check for valid loc

aaron.ballman added subscribers: hans, aaron.ballman. aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM with a minor suggestion to not call `getLocStart()` twice and a formatting fix. I think you should also ping @hans to ge

[PATCH] D41317: Infrastructure for adding C attributes

aaron.ballman closed this revision. aaron.ballman added a comment. I commit in r321763. I changed the name of `IncludeC` to be `AllowInC` at John McCall's suggestion. https://reviews.llvm.org/D41317 ___ cfe-commits mailing list cfe-commits@lists.ll

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

aaron.ballman added inline comments. Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:68 +diag(Matched->getLocStart(), + "the value returned by %0 should normally be used") +<< dyn_cast_or_null(Matched->getCalleeDecl()) "Normally" is

[PATCH] D41720: [clang-tidy] Add a -show-color flag.

aaron.ballman added a comment. This patch is missing test coverage. Comment at: clang-tidy/ClangTidy.cpp:99-103 +if (Context.getOptions().ShowColor.hasValue()) { + DiagOpts->ShowColors = Context.getOptions().ShowColor.getValue(); +} else { + DiagOpts->ShowColo

[PATCH] D41736: make attribute instantiation instantiate all attributes

aaron.ballman added a comment. I don't suppose there is a chance at test coverage for this change? Comment at: include/clang/AST/Attr.h:142 + /// explicitly provided in the current declaration? + bool isInheritEvenIfAlreadyPresent() const { +return InheritEvenIfAlreadyPre

[PATCH] D41736: make attribute instantiation instantiate all attributes

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, thank you! Comment at: include/clang/AST/Attr.h:142 + /// explicitly provided in the current declaration? + bool isInheritEvenIfAlreadyPresent() const {

[PATCH] D41326: [clang-tidy] Added diagnostics about incorrect usage of NOLINT comment

aaron.ballman added inline comments. Comment at: clang-tidy/ClangTidyDiagnosticConsumer.cpp:839-840 +case NolintCommentType::Nolint: + Message = "there is no diagnostics on this line, " +"the NOLINT comment is redundant"; + break; ---

[PATCH] D41039: Add support for attribute "trivial_abi"

aaron.ballman added inline comments. Comment at: include/clang/Basic/Attr.td:1159 +def TrivialABI : InheritableAttr { + let Spellings = [Clang<"trivial_abi">]; + let Subjects = SubjectList<[CXXRecord]>; Would this attribute make sense in C, or does it really on

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

aaron.ballman added inline comments. Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:69 + "the value returned by %0 should normally be used") +<< dyn_cast_or_null(Matched->getCalleeDecl()) +<< Matched->getSourceRange(); khuttun

[PATCH] D41546: [clang-tidy] Adding Fuchsia checker for statically constructed objects

aaron.ballman added inline comments. Comment at: clang-tidy/fuchsia/StaticallyConstructedObjectsCheck.cpp:48 + if (const auto *D = Result.Nodes.getNodeAs("decl")) { +diag(D->getLocStart(), "explicit global static objects are disallowed: if " + "poss

[PATCH] D41740: [clang-tidy] Adding a new bugprone check for streaming objects of type int8_t or uint8_t

aaron.ballman added inline comments. Comment at: clang-tidy/bugprone/BugproneTidyModule.cpp:64 +CheckFactories.registerCheck( +"bugprone-stream-int8"); CheckFactories.registerCheck( I don't think this is a good name for the check, especially beca

[PATCH] D33563: Track whether a unary operation can overflow

aaron.ballman added a comment. Thanks for the review! Comment at: include/clang/AST/Expr.h:1728 + UnaryOperator(Expr *input, Opcode opc, QualType type, ExprValueKind VK, +ExprObjectKind OK, SourceLocation l, bool CanOverflow = false) + : Expr(UnaryOperator

[PATCH] D33563: Track whether a unary operation can overflow

aaron.ballman updated this revision to Diff 128858. aaron.ballman marked 3 inline comments as done. aaron.ballman added a comment. Herald added a subscriber: javed.absar. Updated based on review feedback. https://reviews.llvm.org/D33563 Files: include/clang/AST/Expr.h lib/AST/ASTDumper.cpp

[PATCH] D41815: [clang-tidy] implement check for goto

aaron.ballman added a comment. In https://reviews.llvm.org/D41815#969673, @JonasToth wrote: > In https://reviews.llvm.org/D41815#969626, @xazax.hun wrote: > > > A high level comment: while it is very easy to get all the gotos using > > grep, it is not so easy to get all the labels. So for this c

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

aaron.ballman added inline comments. Comment at: test/clang-tidy/bugprone-unused-return-value.cpp:163 + +void noWarning() { + auto AsyncRetval1 = std::async(increment, 42); khuttun wrote: > aaron.ballman wrote: > > Sorry, I just realized that we're missing a tes

[PATCH] D41815: [clang-tidy] implement check for goto

aaron.ballman added a comment. In https://reviews.llvm.org/D41815#969708, @JonasToth wrote: > > Rather than add a warning for the labels, I would just add a note for the > > label when diagnosing the goto (since the goto has a single target). > > That might lead to existing labels without any go

[PATCH] D41815: [clang-tidy] implement check for goto

aaron.ballman added inline comments. Comment at: clang-tidy/cppcoreguidelines/AvoidGotoCheck.cpp:22 + if (getLangOpts().CPlusPlus) +Finder->addMatcher(gotoStmt().bind("goto"), this); +} JonasToth wrote: > aaron.ballman wrote: > > aaron.ballman wrote: > > > A

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

aaron.ballman added inline comments. Comment at: test/clang-tidy/bugprone-unused-return-value.cpp:163 + +void noWarning() { + auto AsyncRetval1 = std::async(increment, 42); khuttun wrote: > aaron.ballman wrote: > > khuttun wrote: > > > aaron.ballman wrote: > > >

[PATCH] D33563: Track whether a unary operation can overflow

aaron.ballman added inline comments. Comment at: test/Misc/ast-dump-stmt.c:66 + // CHECK:ImplicitCastExpr + // CHECK: DeclRefExpr{{.*}}'T2' 'int' +} efriedma wrote: > aaron.ballman wrote: > > efriedma wrote: > > > What does it mean for bitwise

[PATCH] D33563: Track whether a unary operation can overflow

aaron.ballman updated this revision to Diff 128984. aaron.ballman marked 2 inline comments as done. aaron.ballman added a comment. Updated based on review feedback. https://reviews.llvm.org/D33563 Files: include/clang/AST/Expr.h lib/AST/ASTDumper.cpp lib/AST/ASTImporter.cpp lib/AST/Expr

[PATCH] D33563: Track whether a unary operation can overflow

aaron.ballman closed this revision. aaron.ballman added a comment. Thanks! Committed in r322074. https://reviews.llvm.org/D33563 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41553: Support parsing double square-bracket attributes in ObjC

aaron.ballman added inline comments. Comment at: include/clang/Basic/Attr.td:239 + bit IncludeC = includeC; +} rjmccall wrote: > aaron.ballman wrote: > > rjmccall wrote: > > > I have no objection to allowing ObjC attributes to be spelled in > > > [[clang::obj

[PATCH] D41897: Fixing a crash in Sema.

aaron.ballman added reviewers: rsmith, aaron.ballman. aaron.ballman added inline comments. Comment at: Sema/SemaDeclCXX.cpp:2426 +// Skip all dependent types in templates being used as base specifiers. +// Checks below assume that base specifier is a CXXRecord. +if (B

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

aaron.ballman added inline comments. Comment at: test/clang-tidy/bugprone-unused-return-value.cpp:210 + // test that the check is disabled inside GNU statement expressions + ({ std::async(increment, 42); }); + auto StmtExprRetval = ({ std::async(increment, 42); }); ---

[PATCH] D41546: [clang-tidy] Adding Fuchsia checker for statically constructed objects

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 that I missed, LGTM! Comment at: clang-tidy/fuchsia/StaticallyConstructedObjectsCheck.cpp:51 +const MatchFinder::MatchResu

[PATCH] D41933: handle scoped_lockable objects being returned by value in C++17

aaron.ballman added a comment. I think this approach is a reasonable approximation, but @delesley has the final word. Comment at: lib/Analysis/ThreadSafety.cpp:1994 +static CXXConstructorDecl *findConstructorForByValueReturn(CXXRecordDecl *RD) { + // Prefer a move construct

[PATCH] D41897: Fixing a crash in Sema.

aaron.ballman added inline comments. Comment at: SemaCXX/base-class-ambiguity-check.cpp:1 +// RUN: %clang_cc1 -fsyntax-only %s + aaron.ballman wrote: > This run line isn't testing anything. Since you're trying to ensure this > doesn't crash, I would put `-verify

[PATCH] D41815: [clang-tidy] implement check for goto

aaron.ballman added a comment. In https://reviews.llvm.org/D41815#973260, @JonasToth wrote: > I enhanced the check to do more: > > - check that jumps will only be forward. AFAIK that is required in all > sensefull usecases of goto, is it? > - additionally check for gotos in nested loops. These a

[PATCH] D41933: handle scoped_lockable objects being returned by value in C++17

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, but you should wait for a bit to see if DeLesley has concerns. Comment at: lib/Analysis/ThreadSafety.cpp:1994 +static CXXConstructorDecl *findConstructo

[PATCH] D40580: [clang-tidy] Adding Fuchsia checker for multiple inheritance

aaron.ballman added inline comments. Comment at: clang-tidy/fuchsia/MultipleInheritanceCheck.cpp:60 + // To be an interface, all base classes must be interfaces as well. + for (const auto &I : Node->bases()) { +const auto *Ty = I.getType()->getAs(); rsmith

[PATCH] D43392: [clang-tidy] Add Fuchsia checker for visibility attributes

aaron.ballman added inline comments. Comment at: clang-tidy/fuchsia/AddVisibilityCheck.cpp:52-53 + Finder->addMatcher( + functionDecl(allOf(hasAnyName(SmallVector(Names.begin(), + Names.end())), +

[PATCH] D43500: [clang-tidy]: modernize-use-default-member-init: Remove trailing comma and colon.

aaron.ballman added inline comments. Comment at: unittests/clang-tidy/ClangTidyTest.h:159-160 + CleannedReplacements = std::move(FormattedReplacements); + if (!CleannedReplacements) +llvm_unreachable("!CleannedReplacements"); +} else { jdeme

[PATCH] D43778: [clang-tidy] Adding RestrictIncludes check to Fuchsia module

aaron.ballman added inline comments. Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.cpp:37-39 +IncludeDirective(SourceLocation Loc_, CharSourceRange Range_, + StringRef Filename_) +: Loc(Loc_), Range(Range_), Filename(Filename_) {} --

[PATCH] D43748: [Attr] Fix paren, comma, and omitted arg printing in some cases

aaron.ballman added inline comments. Comment at: test/Sema/attr-print.c:12 +short arr[3] __attribute__((aligned)); + // CHECK: void foo() __attribute__((const)); Please add a test showing that `objc_bridge_related` isn't mangled by `-ast-print`. =

[PATCH] D41880: Adding nocf_check attribute for cf-protection fine tuning

aaron.ballman added a reviewer: rsmith. aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Aside from some minor testing nits, I think this LGTM. However, you should wait to see if Richard has comments as well before committing.

[PATCH] D43748: [Attr] Fix paren, comma, and omitted arg printing in some cases

aaron.ballman added inline comments. Comment at: test/Sema/attr-print.c:12 +short arr[3] __attribute__((aligned)); + // CHECK: void foo() __attribute__((const)); jdenny wrote: > aaron.ballman wrote: > > Please add a test showing that `objc_bridge_related` isn't

[PATCH] D43766: [clang-tidy][modernize-make-unique] Checks c++14 flag before using std::make_unique

aaron.ballman added inline comments. Comment at: clang-tidy/modernize/MakeSmartPtrCheck.cpp:64 +bool MakeSmartPtrCheck::isLanguageVersionSupported(const LangOptions & LangOpts) const +{ The formatting here is wrong. https://reviews.llvm.org/D43766 ___

[PATCH] D43778: [clang-tidy] Adding RestrictIncludes check to Fuchsia module

aaron.ballman added inline comments. Comment at: clang-tidy/fuchsia/RestrictIncludesCheck.cpp:42-44 +SourceLocation Loc; ///< '#' location in the include directive +CharSourceRange Range; ///< SourceRange for the file name +std::string Filename; ///< Filename a

[PATCH] D43747: [Attr] Fix pointer_with_type_tag assert fail for variadic

aaron.ballman added a comment. In https://reviews.llvm.org/D43747#1021209, @jdenny wrote: > In https://reviews.llvm.org/D43747#1018844, @aaron.ballman wrote: > > > Committed in r326057. > > > > If you're not already on IRC, I would recommend joining the #llvm channel > > on OFTC so that you can

[PATCH] D43748: [Attr] Fix paren, comma, and omitted arg printing in some cases

aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! I can commit on your behalf, or, if you plan to continue submitting patches, you can ask for commit privileges (http://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-a

[PATCH] D43748: [Attr] Fix paren, comma, and omitted arg printing in some cases

aaron.ballman closed this revision. aaron.ballman added a comment. Committed in r326266. https://reviews.llvm.org/D43748 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43867: Rename a few checks from misc- to bugprone-.

aaron.ballman added a comment. When we do this sort of move, do we want to keep the check under its old name for a deprecation period so that we are less likely to break automated scripts and whatnot? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D43867

[PATCH] D43847: [clang-tidy] Add check: replace string::find(...) == 0 with absl::StartsWith

aaron.ballman added a comment. I need a bit more context because I'm unfamiliar with `absl`. What is this module's intended use? Comment at: clang-tidy/absl/StringFindStartswithCheck.cpp:17-19 + auto stringClassMatcher = anyOf(cxxRecordDecl(hasName("string")), +

<    1   2   3   4   5   6   7   8   9   10   >