[PATCH] D28451: [AVR] Add support for the 'interrupt' and 'naked' attributes

2017-02-07 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D28451 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D29267: [clang-tidy] safety-no-assembler

2017-02-06 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. There were review comments still outstanding when you commit the patch. Can you please address those post-commit? Repository: rL LLVM https://reviews.llvm.org/D29267 ___ cfe-commits mailing list

[PATCH] D29267: [clang-tidy] safety-no-assembler

2017-02-06 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/safety/NoAssemblerCheck.cpp:13 +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Lex/Lexer.h" + Is this include needed? Comment at:

[PATCH] D29267: [clang-tidy] safety-no-assembler

2017-02-05 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: clang-tools-extra/clang-tidy/safety/NoAssemblerCheck.cpp:32 + + diag(ASM->getAsmLoc(), "'%0' is an inline assembler statement") << SourceText; +} The diagnostic text doesn't help the user to understand why the

[PATCH] D29393: [clang-tidy] Don't warn about call to unresolved operator*

2017-02-05 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM with a minor nit that can be fixed without further review. Comment at: test/clang-tidy/misc-unconventional-assign-operator.cpp:92 +enum E { e }; +E

[PATCH] D28451: [AVR] Add support for the 'interrupt' and 'naked' attributes

2017-02-05 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. Just one more issue due to the nature of needing a custom parsed attribute. Comment at: lib/Sema/SemaDeclAttr.cpp:5130 +static void handleAVRInterruptAttr(Sema , Decl *D, const AttributeList ) { + if (!isFunctionOrMethod(D)) { +

[PATCH] D28768: [clang-tidy] Add check 'modernize-return-braced-init-list'

2017-02-05 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: clang-tidy/modernize/ReturnBracedInitListCheck.cpp:59 + // Skip explicit construcotrs. + if (MatchedConstructExpr->getConstructor()->isExplicit()) +return; Can't this be handled as part of the matcher?

[PATCH] D29267: [clang-tidy] safety-no-assembler

2017-02-06 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D29267#667487, @alexfh wrote: > I wonder whether there's a compiler diagnostic for this purpose. Compiler > diagnostics are more efficient at reaching users and should be preferred > where they are appropriate (this seems like one of

[PATCH] D28451: [AVR] Add support for the 'interrupt' and 'naked' attributes

2017-02-05 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. The new tests aren't really what I had in mind. The codegen tests that should be sema tests can just be rolled into your existing sema tests, by the way. Comment at: lib/Sema/SemaDeclAttr.cpp:5137 + if (!checkAttributeNumArgs(S, Attr, 0)) +

[PATCH] D28520: Disable -Wthread-safety-analysis for some functions in __thread_support

2017-01-23 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D28520#652607, @dim wrote: > In https://reviews.llvm.org/D28520#648880, @delesley wrote: > > > Sorry about the slow response. My main concern here is that the thread > > safety analysis was designed for use with a library that wraps

[PATCH] D28451: [AVR] Add support for the 'interrupt' and 'naked' attributes

2017-01-23 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: lib/Sema/SemaDeclAttr.cpp:5158 + case llvm::Triple::avr: +handleAVRInterruptAttr(S, D, Attr); +break; aaron.ballman wrote: > Just call `handleSimpleAttribute()` instead. Since this is no longer truly a

[PATCH] D27621: [clang-tidy] check to find declarations declaring more than one name

2017-01-23 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:37-38 + const auto *DeclStatement = Result.Nodes.getNodeAs("declstmt"); + if (!DeclStatement) +return; + Is there a case where this could happen? I would

[PATCH] D28520: Disable -Wthread-safety-analysis for some functions in __thread_support

2017-01-26 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! Thank you for being patient while we figured this out. :-) https://reviews.llvm.org/D28520 ___ cfe-commits mailing list

[PATCH] D28889: Change where we handle arg-dependent diagnose_if attributes

2017-01-27 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. I don't see anything that looks amiss, but you should wait for @rsmith to approve. Comment at: lib/Sema/SemaChecking.cpp:2520 + +// TODO: Call can technically be a const CallExpr, but const_casting feels ugly, +// and I really don't want to

[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications

2017-01-27 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. There were some issues with failing tests that caused this commit to need to be reverted in r293267 See for instance: http://lab.llvm.org:8011/builders/clang-cmake-aarch64-full/builds/1039 Eugene Zelenko also had some small fixes you might want to incorporate as

[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications

2017-01-26 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D20693#658087, @hintonda wrote: > Great, thanks. Could you commit for me? Certainly! I've commit in r293217. https://reviews.llvm.org/D20693 ___ cfe-commits mailing list

[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications

2017-01-26 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. Yes, this LGTM as well. Thank you for working on this! https://reviews.llvm.org/D20693 ___ cfe-commits mailing list

[PATCH] D29819: Introduce an 'external_source_symbol' attribute that describes the origin and the nature of a declaration

2017-02-21 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: include/clang/Basic/AttrDocs.td:1007 + +defined_in=\ *string-literal* + The name of the source container in which the declaration was defined. The arphaman wrote: > aaron.ballman wrote: > > Would this hold

[PATCH] D30009: Add support for '#pragma clang attribute'

2017-02-21 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: include/clang/Basic/Attr.td:308-311 + // - An attribute requires delayed parsing (LateParsed is on) + // - An attribute has no GNU/CXX11 spelling + // - An attribute has no subject list + // - An attribute derives from a

[PATCH] D30032: Process attributes 'ifunc' and 'alias' when checking for redefinition

2017-02-17 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D30032 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D30009: Add support for '#pragma clang attribute'

2017-02-16 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: include/clang/Basic/Attr.td:308-311 + // - An attribute requires delayed parsing (LateParsed is on) + // - An attribute has no GNU/CXX11 spelling + // - An attribute has no subject list + // - An attribute derives from a

[PATCH] D29692: [clang-tidy] add check modernize-use-const-instead-of-define

2017-02-17 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D29692#676559, @AlexanderLanin wrote: > Thanks for the feedback. As I'm new to this I cannot say whether checking > only the file in question fits better with clang-tidy’s policy or not. > Also, I’m not sure about ODR. Of course, it’s

[PATCH] D29868: Recover more gracefully when __declspec is not supported as a keyword

2017-02-11 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman created this revision. In r238238, we removed __declspec as a universally-accepted keyword -- instead, it is only enabled as a supported keyword when -fms-extensions or -fdeclspec is passed to the driver. However, this had an unfortunate side-effect in that it made for bad

[PATCH] D29839: [clang-tidy] New misc-istream-overflow check

2017-02-12 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D29839#674301, @xazax.hun wrote: > Shouldn't this be a path sensitive check within the clang static analyzer > instead? So branches are properly handled and interprocedural analysis is > done. I agree; I think this check should be

[PATCH] D29692: [clang-tidy] add check modernize-use-const-instead-of-define

2017-02-12 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. One big concern I have with this is the potential to introduce ODR violations into the user's code. We may want to limit this check to only trigger on macros that are defined in the primary source file of the TU rather than something in the include stack. One

[PATCH] D29685: Lit C++11 Compatibility - Function Attributes

2017-02-12 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a reviewer: delesley. aaron.ballman added a subscriber: delesley. aaron.ballman added a comment. The changes to the format string look good to me, but the changes to the thread-safety attributes do not make sense. I *think* those are indicative of a bug with thready safety

[PATCH] D29868: Recover more gracefully when __declspec is not supported as a keyword

2017-02-12 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: lib/Parse/ParseDecl.cpp:2973 + // recognize that scenario and recover gracefully. + if (!getLangOpts().MicrosoftExt && Tok.is(tok::identifier) && + Tok.getIdentifierInfo()->getName().equals("__declspec")) {

[PATCH] D29868: Recover more gracefully when __declspec is not supported as a keyword

2017-02-12 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman updated this revision to Diff 88122. aaron.ballman marked an inline comment as done. aaron.ballman added a comment. Correcting review feedback. https://reviews.llvm.org/D29868 Files: include/clang/Basic/DiagnosticParseKinds.td lib/Parse/ParseDecl.cpp

[PATCH] D28768: [clang-tidy] Add check 'modernize-return-braced-init-list'

2017-02-12 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. I found a few more small nits, but basically LGTM. Comment at: clang-tidy/modernize/ReturnBracedInitListCheck.cpp:70 + + for (int I = 0, NumParams = MatchedFunctionDecl->getNumParams(); + I < NumParams; ++I) { `I` should

[PATCH] D29868: Recover more gracefully when __declspec is not supported as a keyword

2017-02-13 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman marked an inline comment as done. aaron.ballman added inline comments. Comment at: lib/Parse/ParseDecl.cpp:2989 + + Diag(Loc, diag::err_ms_attributes_not_enabled); + continue; majnemer wrote: > aaron.ballman wrote: > > compnerd

[PATCH] D29819: Introduce an 'external_source_symbol' attribute that describes the origin and the nature of a declaration

2017-02-10 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman requested changes to this revision. aaron.ballman added inline comments. This revision now requires changes to proceed. Comment at: include/clang/Basic/Attr.td:532 + let Spellings = [GNU<"external_source_symbol">, + CXX11<"gnu",

[PATCH] D23421: [Clang-tidy] CERT-MSC53-CPP (checker for std namespace modification)

2017-02-14 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: clang-tidy/cert/DontModifyStdNamespaceCheck.cpp:34 + + diag(N->getLocation(), + "modification of '%0' namespace can result in undefined behavior") I think this will still trigger false positives because

[PATCH] D29868: Recover more gracefully when __declspec is not supported as a keyword

2017-02-14 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman closed this revision. aaron.ballman added a comment. Commit in r295114. https://reviews.llvm.org/D29868 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D23421: [Clang-tidy] CERT-DCL58-CPP (checker for std namespace modification)

2017-02-15 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: clang-tidy/cert/DontModifyStdNamespaceCheck.cpp:28 + anyOf(hasName("std"), hasName("posix")), + has(decl(unless(cxxRecordDecl(isExplicitTemplateSpecialization()) + .bind("nmspc"), I

[PATCH] D29839: [clang-tidy] New misc-istream-overflow check

2017-02-13 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D29839#674714, @cryptoad wrote: > So if I understand correctly, the consensus is to abandon this and rewrite it > to be part of the clang static analyzer? That's correct. https://reviews.llvm.org/D29839

[PATCH] D29868: Recover more gracefully when __declspec is not supported as a keyword

2017-02-13 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman updated this revision to Diff 88198. aaron.ballman added a comment. Fixed review feedback https://reviews.llvm.org/D29868 Files: include/clang/Basic/DiagnosticParseKinds.td lib/Parse/ParseDecl.cpp test/Parser/declspec-recovery.c test/Parser/declspec-supported.c Index:

[PATCH] D29868: Recover more gracefully when __declspec is not supported as a keyword

2017-02-13 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman marked 3 inline comments as done. aaron.ballman added inline comments. Comment at: lib/Parse/ParseDecl.cpp:2989 + + Diag(Loc, diag::err_ms_attributes_not_enabled); + continue; compnerd wrote: > aaron.ballman wrote: > > compnerd

[PATCH] D28166: Properly merge K functions that have attributes

2017-02-11 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman closed this revision. aaron.ballman added a comment. Committed in r294861. https://reviews.llvm.org/D28166 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D29685: Lit C++11 Compatibility - Function Attributes

2017-02-13 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D29685#675306, @tigerleapgorge wrote: > @aaron.ballman Thank you for the code review. I take it I can commit the > first 2 tests? Yes, the first two tests are good to commit. https://reviews.llvm.org/D29685

[PATCH] D30009: Add support for '#pragma clang attribute'

2017-02-16 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: docs/LanguageExtensions.rst:2418 +In general, the attributes are applied to a declaration only when there would +have been no error or warning for that attribute if it was specified explicitly. +An attribute is applied to each

[PATCH] D23421: [Clang-tidy] CERT-DCL58-CPP (checker for std namespace modification)

2017-02-15 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, thank you for working on this! https://reviews.llvm.org/D23421 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D28768: [clang-tidy] Add check 'modernize-return-braced-init-list'

2017-02-15 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. There's one small wording nit with the diagnostic, but once that's resolved, LGTM as well. Comment at: clang-tidy/modernize/ReturnBracedInitListCheck.cpp:65 + + auto Diag = diag(Loc, "to avoid repeating the

[PATCH] D28729: [clang-tidy] Add -enable-alpha-checks command

2017-01-15 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D28729#646876, @Prazek wrote: > In https://reviews.llvm.org/D28729#646758, @aaron.ballman wrote: > > > In https://reviews.llvm.org/D28729#646560, @alexfh wrote: > > > > > In https://reviews.llvm.org/D28729#646555, @aaron.ballman wrote: >

[PATCH] D28768: [clang-tidy] Add check 'modernize-return-braced-init-list'

2017-01-16 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: clang-tidy/modernize/ReturnBracedInitListCheck.cpp:60 + auto Diag = + diag(Loc, "use braced initializer list for constructing return types"); + This diagnostic doesn't really tell the user what's wrong with

[PATCH] D28850: [docs] Tell Doxygen to expand LLVM_ALIGNAS to nothing

2017-01-18 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, though I freely admit that I am no doxygen expert (from reading the docs, this looks like the sensible approach though). https://reviews.llvm.org/D28850

[PATCH] D28902: [Sema] Reword unused lambda capture warning

2017-01-19 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D28902 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D28924: [AST Printer] Print attributes on enum constants

2017-01-19 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, though if you wanted to add one more test with a C++ attribute (deprecated would work in C++14 mode), that would not make me sad. Something like: enum [[deprecated]] E

[PATCH] D28924: [AST Printer] Print attributes on enum constants

2017-01-19 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D28924#651139, @jordan_rose wrote: > Interestingly, this case doesn't actually work yet; we unconditionally print > enum attributes after the close-brace even though that's not valid for C++ > attributes. Do you think I should change

[PATCH] D30327: [Sema] Improve side effect checking for unused-lambda-capture warning

2017-02-28 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! In https://reviews.llvm.org/D30327#688254, @malcolm.parsons wrote: > I found this FIXME comment in `Expr::HasSideEffects()`: > > case LambdaExprClass: { > const

[PATCH] D28266: Transparent_union attribute should be possible in front of union (rework)

2017-02-28 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, with one formatting nit. Comment at: lib/Sema/SemaDeclAttr.cpp:6201 +void Sema::ProcessDeclAttributeDelayed(Decl *D, const AttributeList *AttrList) { +

[PATCH] D30166: Honor __unaligned in codegen for declarations and expressions

2017-02-28 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. I think this LGTM, but you should wait for confirmation from one of the other reviewers before committing. https://reviews.llvm.org/D30166

[PATCH] D30327: [Sema] Improve side effect checking for unused-lambda-capture warning

2017-02-27 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: lib/Sema/SemaLambda.cpp:1458 + + return false; +} I think this is missing one more case: capturing a `volatile` variable by copy does have a side effect in that the variable is read when the capture occurs.

[PATCH] D28266: Transparent_union attribute should be possible in front of union (rework)

2017-02-27 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. Some minor nits, but I think this is generally correct. Comment at: include/clang/Sema/Sema.h:3059 void ProcessDeclAttributes(Scope *S, Decl *D, const Declarator ); + // Helper for delayed proccessing TransparentUnion attribute. + void

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

2017-02-27 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:25 +static const StringRef ReplaceMessage = +"do not use 'random_shuffle'. It is deprecated and replaced by 'shuffle'."; + Diagnostics are not full sentences.

[PATCH] D30192: [Sema] Detecting more array index out of bounds

2017-02-27 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. A few minor nits that can be resolved when you commit, but aside from those, LGTM. Comment at: lib/Sema/SemaChecking.cpp:10613 + case

[PATCH] D29819: Introduce an 'external_source_symbol' attribute that describes the origin and the nature of a declaration

2017-02-27 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: lib/Parse/ParseDecl.cpp:1161 +} else { + assert(Keyword == Ident_defined_in); + if (HadDefinedIn) { Add a string literal to the assert? Comment at:

[PATCH] D30166: Honor __unaligned in codegen for declarations and expressions

2017-02-27 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a reviewer: majnemer. aaron.ballman added inline comments. Comment at: include/clang/AST/ASTContext.h:1909 +// the unqualified type. +if (T.getQualifiers().hasUnaligned()) + TI.Align = getCharWidth(); This makes me a bit

[PATCH] D26800: [Sema] Make __attribute__((notnull)) inheritable through function parameters

2016-12-16 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. Modulo the comment from @ahatanak, I think this looks good. Once you have addressed that comment, I can commit for you. https://reviews.llvm.org/D26800 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D14274: Add alloc_size attribute to clang

2016-12-16 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. I still think that this looks good, so if @rsmith doesn't comment over the weekend, you can go ahead and commit on Monday -- we can handle any feedback in post-commit review. Comment at: include/clang/Basic/AttrDocs.td:240 + Specifically,

[PATCH] D27815: [clang-tidy] Add obvious module for obvious bugs

2016-12-16 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. I am really not keen on the name "obvious" for this module. What is obvious to one person is not always obvious to another. Also, if the checks are finding *obvious bugs*, then that suggests they should be implemented in the clang frontend rather than a tool that

[PATCH] D27815: [clang-tidy] Add obvious module for obvious bugs

2016-12-16 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D27815#625271, @Prazek wrote: > In https://reviews.llvm.org/D27815#625102, @aaron.ballman wrote: > > > I am really not keen on the name "obvious" for this module. What is obvious > > to one person is not always obvious to another. Also,

[PATCH] D26453: [clang-tidy] Remove duplicated check from move-constructor-init

2016-12-16 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D26453 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications

2017-01-10 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman requested changes to this revision. aaron.ballman added inline comments. Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:41 +void UseNoexceptCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus) +return;

[PATCH] D28260: Add an argumentsAre matcher

2017-01-10 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman requested changes to this revision. aaron.ballman added a reviewer: aaron.ballman. aaron.ballman added a comment. This revision now requires changes to proceed. You should also regenerate the HTML matcher documentation as part of this patch. Comment at:

[PATCH] D28260: Add an argumentsAre matcher

2017-01-10 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. Given that this can't be expressed as a dynamic matcher, I'm wondering what the use case is for the matcher. Do you have a specific need for this functionality, or do you see this being generally useful? https://reviews.llvm.org/D28260

[PATCH] D28451: [AVR] Add support for the 'interrupt' and 'naked' attributes

2017-01-10 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed. The patch is also missing Sema tests that ensure the attributes are properly diagnosed when applied to something other than a function, a target other than AVR,

[PATCH] D20428: Tracking exception specification source locations

2017-01-10 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D20428#641416, @hintonda wrote: > Aaron, I've got this version in a branch, so if you like, I'd be happy to > apply Malcolm's suggestions. Please do; they're good suggestions. https://reviews.llvm.org/D20428

[PATCH] D28467: [Sema] Add warning for unused lambda captures

2017-01-13 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D28467#645348, @malcolm.parsons wrote: > http://lab.llvm.org:8011/builders/sanitizer-ppc64be-linux/builds/1122/steps/bootstrap%20clang/logs/stdio > > >

[PATCH] D28703: [clang] Emit `diagnose_if` warnings from system headers

2017-01-13 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM! https://reviews.llvm.org/D28703 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D28729: [clang-tidy] Add -enable-alpha-checks command

2017-01-14 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a reviewer: aaron.ballman. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM with a few minor nits, but in the future, please provide some summary of what your patch is going to do rather than leave it

[PATCH] D28672: [ASTMatchers] update doc by running dump_ast_matchers.py

2017-01-13 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. I'm not seeing anything wrong, per se, but why has so much of this file changed recently? https://reviews.llvm.org/D28672 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D28520: Disable -Wthread-safety-analysis for some functions in __thread_support

2017-01-13 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. I feel like I must be missing something; why is this disabling rather than specifying the thread safety behavior? e.g., `__libcpp_mutex_lock()` specifying that it acquires the capability and `__libcpp_mutex_unlock()` specifying that it releases it?

[PATCH] D28467: [Sema] Add warning for unused lambda captures

2017-01-13 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: test/SemaCXX/warn-unused-lambda-capture.cpp:86 + auto explicit_by_value_unused_sizeof = [i] { return sizeof(i); }; // expected-warning{{lambda capture 'i' is not required to be captured for use in an unevaluated context}} +

[PATCH] D28467: [Sema] Add warning for unused lambda captures

2017-01-10 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: include/clang/Sema/ScopeInfo.h:457 +/// lambda. +bool Used = false; + arphaman wrote: > malcolm.parsons wrote: > > Should this be moved into one of the `PointerIntPair`s? > I'm not sure.. If we needed

[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications

2017-01-10 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: docs/clang-tidy/checks/modernize-use-noexcept.rst:6-8 +The check converts dynamic exception specifications, e.g., +``throw()``, ``throw([,...])``, or ``throw(...)``, to +``noexcept``, ``noexcept(false)``, blank, or a user defined

[PATCH] D28467: [Sema] Add warning for unused lambda captures

2017-01-12 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM; this is an awesome new diagnostic, thank you for working on it! Comment at: test/SemaCXX/warn-unused-lambda-capture.cpp:26 + auto

[PATCH] D28451: [AVR] Add support for the 'interrupt' and 'naked' attributes

2017-01-12 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: include/clang/Basic/Attr.td:488 + +def AVRSignal : InheritableAttr, TargetSpecificAttr { + let Spellings = [GNU<"signal">]; dylanmckay wrote: > aaron.ballman wrote: > > Does this attribute appertain to any

[PATCH] D20428: Tracking exception specification source locations

2017-01-12 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D20428#644007, @hintonda wrote: > Sorry, but I do not have commit access. Ah! My apologies, I thought you did. @malcolm.parsons, can you perform the commit? I'm traveling currently and can't do it myself right now.

[PATCH] D28467: [Sema] Add warning for unused lambda captures

2017-01-12 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: include/clang/Basic/DiagnosticSemaKinds.td:319 InGroup, DefaultIgnore; +def warn_unused_lambda_capture: Warning<"lambda capture %0 is not odr-used">, + InGroup, DefaultIgnore; We do not use the term "odr-use"

[PATCH] D20428: Tracking exception specification source locations

2017-01-12 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D20428#643339, @hintonda wrote: > If you want this included in the 4.0 release, it needs to get in before they > branch tomorrow. Continues to LGTM. https://reviews.llvm.org/D20428 ___

[PATCH] D28467: [Sema] Add warning for unused lambda captures

2017-01-13 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: test/SemaCXX/warn-unused-lambda-capture.cpp:86 + auto explicit_by_value_unused_sizeof = [i] { return sizeof(i); }; // expected-warning{{lambda capture 'i' is not required to be captured for use in an unevaluated context}} +

[PATCH] D28729: [clang-tidy] Add -enable-alpha-checks command

2017-01-15 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman requested changes to this revision. aaron.ballman added a comment. In https://reviews.llvm.org/D28729#646560, @alexfh wrote: > In https://reviews.llvm.org/D28729#646555, @aaron.ballman wrote: > > > In https://reviews.llvm.org/D28729#646548, @alexfh wrote: > > > > > As discussed

[PATCH] D20428: Tracking exception specification source locations

2017-01-10 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. I think that these changes look good to me, and I noticed Richard signed off on the other review for the exception spec changes (thank you for working on that!), so I think this is ready to commit. In https://reviews.llvm.org/D20428#641068, @hintonda wrote: >

[PATCH] D28520: Disable -Wthread-safety-analysis for some functions in __thread_support

2017-01-10 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a reviewer: delesley. aaron.ballman added a comment. Alternatively, these functions could be given the proper thread safety annotations, couldn't they? https://reviews.llvm.org/D28520 ___ cfe-commits mailing list

[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications

2017-01-10 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:68 + this); +} + hintonda wrote: > aaron.ballman wrote: > > Also missing: typedefs and using declarations. > As far as I know, it isn't legal to add dynamic exception

[PATCH] D28520: Disable -Wthread-safety-analysis for some functions in __thread_support

2017-01-10 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D28520#641536, @dim wrote: > In https://reviews.llvm.org/D28520#641534, @aaron.ballman wrote: > > > Alternatively, these functions could be given the proper thread safety > > annotations, couldn't they? > > > Aha, I was not aware of the

[PATCH] D27621: [clang-tidy] check to find declarations declaring more than one name

2017-01-05 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:34 + // a tag declaration (e.g. struct, class etc.): + // class A { } Object1, Object2; <-- won't be matched + Finder->addMatcher( firolino wrote: >

[PATCH] D27424: Add the diagnose_if attribute to clang.

2017-01-05 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D27424#636496, @george.burgess.iv wrote: > Do we have a page that details when we should/shouldn't use `auto`? I was > under the impression that it was preferred only in cases where the type's > spelled out (e.g. `cast`, ...). (To be

[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications

2017-01-06 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D20693#638062, @mgehre wrote: > Hi, > this is a good idea for a check. > > I would prefer when the FixIt just removes throw(A,B) specifier, instead of > replacing it by noexcept(false), > because noexcept(false) means the same things

[PATCH] D28180: Fixing small errors in libAST Matcher Tutorial.

2016-12-30 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, thank you for the documentation fix! https://reviews.llvm.org/D28180 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D28166: Properly merge K functions that have attributes

2017-01-03 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D28166#633643, @rsmith wrote: > In https://reviews.llvm.org/D28166#633621, @aaron.ballman wrote: > > > Do you think this patch should be gated on (or perhaps combined with) a fix > > for the lowering bug, or do you think this patch is

[PATCH] D27621: [clang-tidy] check to find declarations declaring more than one name

2017-01-03 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:34 + // a tag declaration (e.g. struct, class etc.): + // class A { } Object1, Object2; <-- won't be matched + Finder->addMatcher( firolino wrote: >

[PATCH] D28321: [CUDA] Add __declspec spellings for CUDA attributes.

2017-01-04 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Attr.td:604 +// CUDA attributes are spelled __attribute__((attr)) or __declspec(__attr__). + jlebar wrote: > aaron.ballman wrote: > > jlebar wrote: > > > aaron.ballman wrote: > > > >

[PATCH] D27621: [clang-tidy] check to find declarations declaring more than one name

2017-01-04 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: clang-tidy/readability/OneNamePerDeclarationCheck.cpp:34 + // a tag declaration (e.g. struct, class etc.): + // class A { } Object1, Object2; <-- won't be matched + Finder->addMatcher( firolino wrote: >

[PATCH] D28287: [clang-tidy] Ignore default arguments in modernize-default-member-init

2017-01-04 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. Btw, when creating a patch, it's helpful to the reviewers (especially ones who are only casually interested) to put some of the context from the bug report into the patch summary rather than only list the PR. Comment at:

[PATCH] D28287: [clang-tidy] Ignore default arguments in modernize-default-member-init

2017-01-04 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added inline comments. Comment at: clang-tidy/modernize/UseDefaultMemberInitCheck.cpp:161 cxxBoolLiteral(), cxxNullPtrLiteralExpr(), implicitValueInitExpr(), -declRefExpr()); +declRefExpr(unless(to(varDecl();

[PATCH] D28166: Properly merge K functions that have attributes

2017-01-03 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman updated this revision to Diff 82985. aaron.ballman marked 3 inline comments as done. aaron.ballman added a comment. Stripped out the codegen changes since @rnk 's commit fixed the issue. https://reviews.llvm.org/D28166 Files: include/clang/AST/Type.h

[PATCH] D28321: [CUDA] Add __declspec spellings for CUDA attributes.

2017-01-04 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. After getting some realtime clarifications in IRC, I now understand better why this is needed. This patch LGTM! The documentation points I raised are still valid, but are by no

[PATCH] D27424: Add the diagnose_if attribute to clang.

2017-01-07 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman accepted this revision. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM, but please point this out in the release notes as well. https://reviews.llvm.org/D27424 ___ cfe-commits mailing list

[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications

2017-01-07 Thread Aaron Ballman via Phabricator via cfe-commits
aaron.ballman added a comment. In https://reviews.llvm.org/D20693#639030, @hintonda wrote: > Matthias, I think you make a good point. While noexcept(expr) is valuable, > noexcept(false) adds no value. Therefore, I think we should do as you > suggest, i.e.: > > s/throw()/noexcept/ >

  1   2   3   4   5   6   7   8   9   10   >