[PATCH] D28451: [AVR] Add support for the 'interrupt' and 'naked' attributes
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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29267: [clang-tidy] safety-no-assembler
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 cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29267: [clang-tidy] safety-no-assembler
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: clang-tools-extra/clang-tidy/safety/NoAssemblerCheck.cpp:36
+void NoAssemblerCheck::check(const MatchFinder::MatchResult ) {
+ Optional ASMLocation;
+ if (const auto *ASM = Result.Nodes.getNodeAs("asm-stmt"))
No need for this to be an `Optional`, is there?
https://reviews.llvm.org/D29267
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29267: [clang-tidy] safety-no-assembler
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 code is being diagnosed. Also, does printing the source text add any clarity? The diagnostic already appears on the line in which the assembly statement appears, and since this is a statement (rather than an expression), it seems unlikely to be useful to repeat that text. https://reviews.llvm.org/D29267 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29393: [clang-tidy] Don't warn about call to unresolved operator*
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 operator*(E, E);
+
You should put a comment near here to explain why this is a necessary part of
the test.
Also, when fixing PRs, it's sometimes helpful to put the entire test into a
namespace named after the PR (so people can do historical tracking a bit more
easily). e.g., `namespace pr31531`. We've not done this often for clang-tidy,
but it's quite common in clang's tests and might be a nice habit to get into --
it helps reduce accidental name collisions as a benefit.
https://reviews.llvm.org/D29393
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28451: [AVR] Add support for the 'interrupt' and 'naked' attributes
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)) {
+S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
Because you have to do custom parsing, you also need to manually check that the
attribute was not given any arguments and diagnose if they are present.
Comment at: test/Sema/avr-interrupt-attr.c:5
+struct a test __attribute__((interrupt)); // expected-warning {{'interrupt'
attribute only applies to functions and methods}}
+
+__attribute__((interrupt)) void food() {}
Please add a test for `__attribute__((interrupt(12)))`, and a test with an
Objective-C method. Similar for `signal`. You should add an ObjC method to the
codegen tests as well.
https://reviews.llvm.org/D28451
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28768: [clang-tidy] Add check 'modernize-return-braced-init-list'
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? Something like `hasDeclaration(cxxConstructorDecl(isExplicit()))` should work. Repository: rL LLVM https://reviews.llvm.org/D28768 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29267: [clang-tidy] safety-no-assembler
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 such cases). I don't think a compiler diagnostic would be appropriate for this. It would be incredibly chatty for people who do need to use the assembler. https://reviews.llvm.org/D29267 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28451: [AVR] Add support for the 'interrupt' and 'naked' attributes
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))
+Attr.setInvalid();
+
This should simply return rather than attempt to attach an invalid attribute to
the declaration (same below).
Comment at: test/CodeGen/avr/attributes/interrupt-no-args.c:3
+
+// CHECK: error: 'interrupt' attribute takes no arguments
+__attribute__((interrupt(12))) void foo(void) { }
This should be a test in Sema, not in CodeGen.
Comment at: test/CodeGen/avr/attributes/interrupt.c:3
+
+// CHECK: define void @foo() #0
+__attribute__((interrupt)) void foo(void) { }
As should this.
Comment at: test/CodeGen/avr/attributes/interrupt.m:1
+// RUN: %clang_cc1 -triple avr-unknown-unknown -x objective-c++ -emit-llvm -o
- %s | FileCheck %s
+
Why objective-c++?
Comment at: test/CodeGen/avr/attributes/interrupt.m:4
+// CHECK: define void @_Z3foov() #0
+void foo() __attribute__((interrupt)) { }
+
This is not an Objective-C method decl, so it doesn't really test anything new.
The test I was envisioning was something like:
```
@interface F // There's probably an expected warning here about a missing base
class
-(void) foo __attribute__((interrupt));
@end
@implementation F
-(void) foo __attribute__((interrupt)) {}
@end
```
Comment at: test/CodeGen/avr/attributes/signal-no-args.c:4
+// CHECK: error: 'signal' attribute takes no arguments
+__attribute__((signal(12))) void foo(void) { }
This should also be a Sema test.
Comment at: test/CodeGen/avr/attributes/signal.m:4
+// CHECK: define void @_Z3foov() #0
+void foo() __attribute__((signal)) { }
+
Same concerns about this not testing anything new as above.
https://reviews.llvm.org/D28451
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28520: Disable -Wthread-safety-analysis for some functions in __thread_support
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 the system > > mutex in a separate Mutex class. We did that specifically to avoid > > breaking anything; code has to opt-in to the static checking by defining > > and using a Mutex class, and the API of that class is restricted to calls > > that can be easily checked via annotations. Including attributes directly > > in the standard library has the potential to cause lots of breakage and > > false positives. > > > Yes, I agree with that. However, on FreeBSD the pthread functions themselves > are already annotated, so the libc++ wrapper functions cause -Werror warnings > during the tests. Therefore one of my suggestions was to explicitly turn off > warnings using `no_thread_safety_analysis` attributes. Is there any > disadvantage in doing that? > > > Is there some way to control the #ifdefs so that the annotations are turned > > off by default for everyone except possibly freebsd, but there's still a > > way to turn them on for users who want to see the warnings? I'm not a > > libcxx expert. > > Yes, that was one of my other suggestions, using `#if defined(__FreeBSD__)` > to limit these attributes to FreeBSD only. We can do that either with the > `no_thread_safety_analysis` attributes, or with the 'real' annotations, > though the latter are not really useful in this case. > > I'm really open to any variant, as long as something that works can get in > before the 4.0.0 release. :) I feel like there's still some confusion here (likely on my part). DeLesley was asking if there was a way to turn this off for everyone *except* FreeBSD (that is user-controllable), but your code turns it off specifically *for* FreeBSD with no option to enable. The part that has me confused is that FreeBSD is annotating their functionality specifically to enable thread safety checking; it seems like turning that checking off rather than supporting it is the wrong way to go. I think that may be why DeLesley was saying to disable the functionality for non-FreeBSD systems while still honoring it on FreeBSD, but if I'm confused, hopefully he'll clarify. https://reviews.llvm.org/D28520 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28451: [AVR] Add support for the 'interrupt' and 'naked' attributes
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 simple attribute (it requires some custom
checking), I think my earlier advice was wrong -- you should split this into a
`handleAVRInterruptAttr()` method. I forgot that you need the custom checking
because the attribute has custom parsing.
Comment at: lib/Sema/SemaDeclAttr.cpp:5721
+ case AttributeList::AT_AVRSignal:
+handleAVRSignalAttr(S, D, Attr);
+break;
aaron.ballman wrote:
> ...same here.
... same here.
Comment at: lib/Sema/SemaDeclAttr.cpp:5145
+if (!isFunctionOrMethod(D)) {
+ S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
+ << "'interrupt'" << ExpectedFunctionOrMethod;
dylanmckay wrote:
> I'm pretty sure that this check shouldn't be necessary, because we define
> `Subjects = [Function]` in TableGen.
>
> Without it though, the warning doesn't appear. Do you know why that is
> @aaron.ballman?
It's because it requires custom parsing.
Comment at: lib/Sema/SemaDeclAttr.cpp:5146
+ S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
+ << "'interrupt'" << ExpectedFunctionOrMethod;
+ return;
The diagnostic and the predicate for it don't match your `Subjects` line.
Should this appertain to ObjC methods? If not, you want to use
`ExpectedFunction` instead. If so, you want to add `ObjCMethod` to the
`Subjects` line.
https://reviews.llvm.org/D28451
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27621: [clang-tidy] check to find declarations declaring more than one name
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 have imagined this as an
`assert()`, if anything.
https://reviews.llvm.org/D27621
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28520: Disable -Wthread-safety-analysis for some functions in __thread_support
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 cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28889: Change where we handle arg-dependent diagnose_if attributes
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 duplicate unwrapCallExpr's logic. No caller really Thank you for this comment; I was about to ask about that very topic. :-D Comment at: lib/Sema/SemaChecking.cpp:11933 } - Unintended change? Comment at: lib/Sema/SemaOverload.cpp:6235 + SmallVector Attrs; + for (const auto *DIA : FD->specific_attrs()) +if (ArgDependent == DIA->getArgDependent()) Braces might make this a bit easier to read due to the multiline if. https://reviews.llvm.org/D28889 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications
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 well before recommitting (see r293234). https://reviews.llvm.org/D20693 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications
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 cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications
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 cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29819: Introduce an 'external_source_symbol' attribute that describes the origin and the nature of a declaration
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 something like a file name? If so, I worry about conflicts
> > between the comma separator and a file name -- you might want to pick a
> > separator that can't be used in a file name (like | or :).
> It could potentially include a filename, yes.
> I don't quite follow your concerns though.. If a comma is in a string literal
> then it's wrapped in quotes. Wouldn't that be enough to distinguish the comma
> separator token from the comma in a filename?
You're correct, I had a brain fart. :-)
Comment at: include/clang/Basic/AttrDocs.td:1005
+language=\ *identifier*
+ The source language in which this declaration was defined.
+
This being an identifier makes me wonder about languages that aren't a single
token. For instance, how do you specify Objective-C or Objective-C++? What
about C++? Visual Basic .NET?
Perhaps this should also be a string literal.
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2713
+ "|classes and enumerations"
+ "|named declarations}1">,
InGroup;
I'm not too keen on this entry, because I'm not certain how many users really
know what a "named declaration" is ("don't all declarations have names?" is a
valid question for this to raise). However, I don't know of a better term to
use, so it may be fine.
Comment at: include/clang/Parse/Parser.h:146
+ /// Identifiers used by the 'external_source_symbol' attribute.
+ IdentifierInfo *Ident_language, *Ident_defined_in,
+ *Ident_generated_declaration;
Can we handle identifiers that have spaces in the name? I'm thinking about the
case where `Ident_defined_in` is something like `"My Long File Name With Spaces
On Windows.cpp"`
We should have something like that as a test to make sure it's handled
properly. Same with odd values for `Ident_language`, like 'c++' or
'Objective-C', etc.
Comment at: lib/Parse/ParseDecl.cpp:1090
+ BalancedDelimiterTracker T(*this, tok::l_paren);
+ if (T.consumeOpen()) {
+Diag(Tok, diag::err_expected) << tok::l_paren;
`expectAndConsume()` instead of manually diagnosing?
Comment at: lib/Parse/ParseDecl.cpp:1160
+ }
+ if (!DefinedInExpr.isUnset()) {
+Diag(KeywordLoc, diag::err_external_source_symbol_duplicate_clause)
As an interesting case: if the attribute has two `defined_in` arguments and the
first one is invalid, they will not get a diagnostic about the second one being
a duplicate. e.g., `__attribute__((external_source_symbol(defined_in =
"1234"udl, defined_in = "1234")))`
Comment at: lib/Parse/ParseDeclCXX.cpp:3818-3819
+ if (ScopeName && (ScopeName->getName() == "gnu" ||
+(ScopeName->getName() == "clang" &&
+ AttrName->isStr("external_source_symbol"
// GNU-scoped attributes have some special cases to handle GNU-specific
I don't really like hard-coding a list of attributes like this. I think you
should handle clang-namespaced attributes with a separate helper function.
Comment at: lib/Sema/SemaDeclAttr.cpp:2414
+ const AttributeList ) {
+ if (!checkAttributeAtLeastNumArgs(S, Attr, 1))
+return;
arphaman wrote:
> aaron.ballman wrote:
> > You should also diagnose if there's more than 3 arguments, no?
> I think an assert would be more appropriate since I only use up to 3
> arguments when creating the attribute, so I wouldn't be able to test the
> diagnostic.
An assert is fine by me, but please add a string literal after the assert to
explain why it fails.
Comment at: lib/Sema/SemaDeclAttr.cpp:2420
+S.Diag(Attr.getLoc(), diag::warn_attribute_wrong_decl_type)
+<< Attr.getName() << /*Named declarations=*/45;
+return;
This is incorrect -- you need to update the enumeration at the end of
AttributeList.h and use an enumerator rather than a magic literal value.
Comment at: test/Parser/attr-external-source-symbol.m:26
+void f6()
+__attribute__((external_source_symbol(defined_in=20))); // expected-error
{{expected string literal for source container name in 'external_source_symbol'
attribute}}
aaron.ballman wrote:
> I think you can currently get away with writing
> `external_source_symbol(generated_declaration, generated_declaration,
> generated_declaration, defined_in="module"))` and other odd combinations.
>
> There should be additional parser tests for totally crazy parsing scenarios.
> You may want to consider running a fuzzer to generate
[PATCH] D30009: Add support for '#pragma clang attribute'
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 StmtAttr or a TypeAttr
arphaman wrote:
> aaron.ballman wrote:
> > arphaman wrote:
> > > aaron.ballman wrote:
> > > > I have strong reservations about this -- users are going to have no
> > > > idea what attributes are and are not supported because they're not
> > > > going to know whether the attribute has a subject list or requires
> > > > delayed parsing. We have a considerable number of attributes for which
> > > > the Subjects line is currently commented out simply because no one has
> > > > bothered to fix that. This means those attributes can't be used with
> > > > this pragma until someone fixes that, but when it happens, they
> > > > magically can be used, which is a good thing. But the converse is more
> > > > problematic -- if there's an existing Subjects line that gets removed
> > > > because a subject is added that is difficult to express in TableGen it
> > > > may break user code.
> > > >
> > > > We can fix the discoverability issues somewhat by updating the
> > > > documentation emitter to spit out some wording that says when an
> > > > attribute is/is not supported by this feature, but that only works for
> > > > attributes which have documentation and so it's not a particularly
> > > > reliable workaround.
> > > > I have strong reservations about this -- users are going to have no
> > > > idea what attributes are and are not supported because they're not
> > > > going to know whether the attribute has a subject list or requires
> > > > delayed parsing. We have a considerable number of attributes for which
> > > > the Subjects line is currently commented out simply because no one has
> > > > bothered to fix that. This means those attributes can't be used with
> > > > this pragma until someone fixes that, but when it happens, they
> > > > magically can be used, which is a good thing. But the converse is more
> > > > problematic -- if there's an existing Subjects line that gets removed
> > > > because a subject is added that is difficult to express in TableGen it
> > > > may break user code.
> > >
> > > That's a good point. I think we can address this problem by creating a
> > > test that verifies the list of attributes that support the pragma. This
> > > would allow us to ensure that no attributes loose the ability to use the
> > > pragma.
> > >
> > > > We can fix the discoverability issues somewhat by updating the
> > > > documentation emitter to spit out some wording that says when an
> > > > attribute is/is not supported by this feature, but that only works for
> > > > attributes which have documentation and so it's not a particularly
> > > > reliable workaround.
> > >
> > > We can enforce the rule that the attributes can only be used with
> > > '#pragma clang attribute' when they're documented. This way we can ensure
> > > that all attributes that can be used with the pragma are explicitly
> > > documented.
> > > That's a good point. I think we can address this problem by creating a
> > > test that verifies the list of attributes that support the pragma. This
> > > would allow us to ensure that no attributes loose the ability to use the
> > > pragma.
> >
> > That would be good.
> >
> > > We can enforce the rule that the attributes can only be used with
> > > '#pragma clang attribute' when they're documented. This way we can ensure
> > > that all attributes that can be used with the pragma are explicitly
> > > documented.
> >
> > This addresses the concern about discoverability, but it worsens the
> > concerns about fragility. The biggest problem is: the user has very little
> > hope of understanding what attributes will apply to what declarations with
> > which version of the compiler they're using. With this sort of thing, the
> > act of us adding documentation can impact the behavior of a user's program
> > from one release to the next.
> >
> > While I can imagine this pragma reducing some amount of code clutter, it is
> > far too "magical" for me to feel comfortable with it (at least in the
> > proposed form). I cannot read the user's source code and understand what
> > attributes are going to be applied to which declarations, and that's not a
> > good story for usability of the feature.
> What about the following idea:
>
> The user has to include a **strict** set of declarations that receive the
> attribute explicitly, e.g.:
>
> ```
> #pragma clang attribute push([[noreturn]], apply_to={ function })
> ```
>
> The compiler then would verify that the set of declarations (in this case
> just `function`) is **strictly** identical to the built-in compiler-defined
> set of declarations that can receive the attribute (i.e.
[PATCH] D30032: Process attributes 'ifunc' and 'alias' when checking for redefinition
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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30009: Add support for '#pragma clang attribute'
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 StmtAttr or a TypeAttr arphaman wrote: > aaron.ballman wrote: > > I have strong reservations about this -- users are going to have no idea > > what attributes are and are not supported because they're not going to know > > whether the attribute has a subject list or requires delayed parsing. We > > have a considerable number of attributes for which the Subjects line is > > currently commented out simply because no one has bothered to fix that. > > This means those attributes can't be used with this pragma until someone > > fixes that, but when it happens, they magically can be used, which is a > > good thing. But the converse is more problematic -- if there's an existing > > Subjects line that gets removed because a subject is added that is > > difficult to express in TableGen it may break user code. > > > > We can fix the discoverability issues somewhat by updating the > > documentation emitter to spit out some wording that says when an attribute > > is/is not supported by this feature, but that only works for attributes > > which have documentation and so it's not a particularly reliable workaround. > > I have strong reservations about this -- users are going to have no idea > > what attributes are and are not supported because they're not going to know > > whether the attribute has a subject list or requires delayed parsing. We > > have a considerable number of attributes for which the Subjects line is > > currently commented out simply because no one has bothered to fix that. > > This means those attributes can't be used with this pragma until someone > > fixes that, but when it happens, they magically can be used, which is a > > good thing. But the converse is more problematic -- if there's an existing > > Subjects line that gets removed because a subject is added that is > > difficult to express in TableGen it may break user code. > > That's a good point. I think we can address this problem by creating a test > that verifies the list of attributes that support the pragma. This would > allow us to ensure that no attributes loose the ability to use the pragma. > > > We can fix the discoverability issues somewhat by updating the > > documentation emitter to spit out some wording that says when an attribute > > is/is not supported by this feature, but that only works for attributes > > which have documentation and so it's not a particularly reliable workaround. > > We can enforce the rule that the attributes can only be used with '#pragma > clang attribute' when they're documented. This way we can ensure that all > attributes that can be used with the pragma are explicitly documented. > That's a good point. I think we can address this problem by creating a test > that verifies the list of attributes that support the pragma. This would > allow us to ensure that no attributes loose the ability to use the pragma. That would be good. > We can enforce the rule that the attributes can only be used with '#pragma > clang attribute' when they're documented. This way we can ensure that all > attributes that can be used with the pragma are explicitly documented. This addresses the concern about discoverability, but it worsens the concerns about fragility. The biggest problem is: the user has very little hope of understanding what attributes will apply to what declarations with which version of the compiler they're using. With this sort of thing, the act of us adding documentation can impact the behavior of a user's program from one release to the next. While I can imagine this pragma reducing some amount of code clutter, it is far too "magical" for me to feel comfortable with it (at least in the proposed form). I cannot read the user's source code and understand what attributes are going to be applied to which declarations, and that's not a good story for usability of the feature. Repository: rL LLVM https://reviews.llvm.org/D30009 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29692: [clang-tidy] add check modernize-use-const-instead-of-define
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 a possibility, but isn’t the > programmer responsible for that? This will be more of an issue as soon as > this check provides a Fix-It solution. It's still an issue without the FixIt because the diagnostic guides the user to do something that may make their well-formed code into ill-formed (no diagnostic required!) code. I think the correct way to handle this is to ensure we don't diagnose macro definitions that exist in a header file, at least for C++ code. > As for the other part, I've checked some guidelines (without any order or > selection process) > MISRA C++: Don’t use `#define`, use `const` variables; Also don’t do math on > enums > CppCoreGuidelines: Don’t use `#define`, use `constexpr` variables > SEI CERT C++: No mention of `#define` as far as I can tell > JSF AV C++: Don’t use `#define`, use `const` variables > HIC++: Don’t use `#define`, use `const` objects (reference to JSF AV C++ and > MISRA C++) > > So basically they're all the same. The only question is `const` vs `constexpr` I don't think the guidance in this check is incorrect, I just think the check is implemented too broadly. As for 'const' vs 'constexpr', such a decision may differ when compiling in C vs C++ mode. https://reviews.llvm.org/D29692 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29868: Recover more gracefully when __declspec is not supported as a keyword
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 diagnostics in the presence of a __declspec when neither of those
flags are passed. For instance:
__declspec(naked) void f(void) {}
clang -fsyntax-only -fno-ms-extensions -Wall -pedantic main.c
main.cpp:1:24: error: parameter named 'f' is missing
__declspec(naked) void f(void) {}
^
main.cpp:1:31: error: expected ';' at end of declaration
__declspec(naked) void f(void) {}
^
;
main.cpp:1:12: warning: parameter 'naked' was not declared, defaulting to type
'int' [-Wpedantic]
__declspec(naked) void f(void) {}
^
main.cpp:1:1: warning: type specifier missing, defaults to 'int'
[-Wimplicit-int]
__declspec(naked) void f(void) {}
This patch addresses the poor diagnostics by noticing when a __declspec has
been used but is not enabled. It eats the attribute, diagnoses the use, and
then attempts to continue parsing.
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: test/Parser/declspec-supported.c
===
--- test/Parser/declspec-supported.c
+++ test/Parser/declspec-supported.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -fms-extensions
-verify %s
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -fdeclspec -verify %s
+// expected-no-diagnostics
+
+__declspec(naked) void f(void) {}
+
+struct S {
+ __declspec(property(get=Getter, put=Setter)) int X;
+ int Y;
+};
Index: test/Parser/declspec-recovery.c
===
--- test/Parser/declspec-recovery.c
+++ test/Parser/declspec-recovery.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -verify %s
+
+__declspec(naked) void f(void) {} // expected-error{{'__declspec' attributes
are not enabled; use '-fdeclspec' or '-fms-extensions' to enable support for
__declspec attributes}}
+
+struct S {
+ __declspec(property(get=Getter, put=Setter)) int X; //
expected-error{{'__declspec' attributes are not enabled; use '-fdeclspec' or
'-fms-extensions' to enable support for __declspec attributes}}
+ int Y;
+};
Index: lib/Parse/ParseDecl.cpp
===
--- lib/Parse/ParseDecl.cpp
+++ lib/Parse/ParseDecl.cpp
@@ -2966,6 +2966,31 @@
if (DS.hasTypeSpecifier())
goto DoneWithDeclSpec;
+ // If the token is an identifier named "__declspec" and Microsoft
+ // extensions are not enabled, it is likely that there will be cascading
+ // parse errors if this really is a __declspec attribute. Attempt to
+ // recognize that scenario and recover gracefully.
+ if (!getLangOpts().MicrosoftExt && Tok.is(tok::identifier) &&
+ Tok.getIdentifierInfo()->getName().equals("__declspec")) {
+// The next token should be an open paren. If it is, eat the entire
+// attribute declaration and continue.
+if (NextToken().is(tok::l_paren)) {
+ // Consume the __declspec identifier.
+ SourceLocation Loc = ConsumeToken();
+
+ // Eat the parens and everything between them.
+ BalancedDelimiterTracker T(*this, tok::l_paren);
+ if (T.consumeOpen()) {
+assert(false && "Not a left paren?");
+return;
+ }
+ T.skipToEnd();
+
+ Diag(Loc, diag::err_ms_attributes_not_enabled);
+ continue;
+}
+ }
+
// In C++, check to see if this is a scope specifier like foo::bar::, if
// so handle it as such. This is important for ctor parsing.
if (getLangOpts().CPlusPlus) {
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -176,6 +176,9 @@
def warn_attribute_no_decl : Warning<
"attribute %0 ignored, because it is not attached to a declaration">,
InGroup;
+def err_ms_attributes_not_enabled : Error<
+ "'__declspec' attributes are not enabled; use '-fdeclspec' or "
+ "'-fms-extensions' to enable support for __declspec attributes">;
def err_expected_method_body : Error<"expected method body">;
def err_declspec_after_virtspec : Error<
"'%0' qualifier may not appear after the virtual specifier '%1'">;
Index: test/Parser/declspec-supported.c
===
--- test/Parser/declspec-supported.c
+++ test/Parser/declspec-supported.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -fms-extensions -verify
[PATCH] D29839: [clang-tidy] New misc-istream-overflow check
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 part of the static analyzer because it is path sensitive if we want it to be particularly useful. As it stands now, it will catch trivial bugs, but by designing it as a clang-tidy check, it isn't easily extensible to catch the bigger bugs across procedures. https://reviews.llvm.org/D29839 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29692: [clang-tidy] add check modernize-use-const-instead-of-define
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 other problem with this advice is that a constant variable isn't always the
same thing as a macro replacement value. For instance, a const variable
requires the ability to take the address of the variable, which means that
space must be reserved for it at runtime. A frequent way around this is to use
enumerator rather than constant variables for integral types. The enumerations
are not prone to ODR violations and enumerators are not things with storage.
Comment at: clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp:41
+
+/// numeric values may have + or - signs in front of them
+/// others like ~ are not so obvious and depend on usage
Make sure the comments have proper capitalization and punctuation, here and
elsewhere.
Comment at: clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp:43
+/// others like ~ are not so obvious and depend on usage
+static bool isReasonableNumberPrefix(const Token ) {
+ return T.isOneOf(tok::plus, tok::minus);
I don't think most of these helper functions really clarify the code all that
much (except for perhaps `isAnyCharLiteral()`, but even that's debatable). I
would just fold these into the code.
Comment at: clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp:83-84
+}
+// numbers may have a prefix, however chars with a prefix are rather
+// strange... let's not touch them
+else if (isAnyParenthesis(Tok) || isAnyNumericLiteral(Tok) ||
I don't really agree with this comment. `L'1'` is a reasonable character
constant and not at all strange. You should add tests for that case and perhaps
clarify the comment.
Comment at: clang-tidy/modernize/UseConstInsteadOfDefineCheck.cpp:86
+else if (isAnyParenthesis(Tok) || isAnyNumericLiteral(Tok) ||
+ (!hasPrefix && isAnyCharLiteral(Tok))) {
+ // prefix shall not be accepted anymore after any number
What about string literals? e.g., `#define NAME "foobar"`
Comment at: clang-tidy/modernize/UseConstInsteadOfDefineCheck.h:19-21
+/// C++ const variables should be preferred over #define statements
+///
+/// const variables obey type checking and scopes
Missing punctuation at the end of the sentences in these comments.
Comment at: docs/clang-tidy/checks/modernize-use-const-instead-of-define.rst:14
+
+ `// calc.h
+ namespace RealCalculation {
Extraneous ` before the comment.
https://reviews.llvm.org/D29692
___
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
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 analysis, but @delesley would have the definitive answer there. I'm not certain how a static function or variable can be safely guarded by an instance member. https://reviews.llvm.org/D29685 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29868: Recover more gracefully when __declspec is not supported as a keyword
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")) {
compnerd wrote:
> Shouldn't this be `getLangOpts().DeclSpecKeyword` since you don't need the
> Microsoft extensions as much as you need the declspec keyword to be
> processed. Having `MicrosoftExt` implicitly enable `DeclSpecKeyword` (just
> as borland mode enables it as well). I suppose that it would fail anyways as
> `Tok.is(tok::identifier)` would fail.
That's a good point, I've corrected.
Comment at: lib/Parse/ParseDecl.cpp:2989
+
+ Diag(Loc, diag::err_ms_attributes_not_enabled);
+ continue;
compnerd wrote:
> I think that we want to emit the diagnostic even if there is no parenthesis
> as `__declspec` is a reserved identifier, and we would normally diagnose the
> bad `__declspec` (expected '(' after '__declspec').
Yes, but it could also lead to a rejecting code that we used to accept and
properly handle when __declspec is an identifier rather than a keyword. e.g.,
```
struct __declspec {};
__declspec some_func(void);
```
By looking for the paren, we run less risk of breaking working code, even if
that code abuses the implementation namespace (after all, __declspec it not a
keyword in this scenario).
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] D29868: Recover more gracefully when __declspec is not supported as a keyword
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
test/Parser/declspec-recovery.c
test/Parser/declspec-supported.c
Index: test/Parser/declspec-supported.c
===
--- test/Parser/declspec-supported.c
+++ test/Parser/declspec-supported.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -fms-extensions
-verify %s
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -fdeclspec -verify %s
+// expected-no-diagnostics
+
+__declspec(naked) void f(void) {}
+
+struct S {
+ __declspec(property(get=Getter, put=Setter)) int X;
+ int Y;
+};
Index: test/Parser/declspec-recovery.c
===
--- test/Parser/declspec-recovery.c
+++ test/Parser/declspec-recovery.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -verify %s
+
+__declspec(naked) void f(void) {} // expected-error{{'__declspec' attributes
are not enabled; use '-fdeclspec' or '-fms-extensions' to enable support for
__declspec attributes}}
+
+struct S {
+ __declspec(property(get=Getter, put=Setter)) int X; //
expected-error{{'__declspec' attributes are not enabled; use '-fdeclspec' or
'-fms-extensions' to enable support for __declspec attributes}}
+ int Y;
+};
Index: lib/Parse/ParseDecl.cpp
===
--- lib/Parse/ParseDecl.cpp
+++ lib/Parse/ParseDecl.cpp
@@ -2966,6 +2966,31 @@
if (DS.hasTypeSpecifier())
goto DoneWithDeclSpec;
+ // If the token is an identifier named "__declspec" and Microsoft
+ // extensions are not enabled, it is likely that there will be cascading
+ // parse errors if this really is a __declspec attribute. Attempt to
+ // recognize that scenario and recover gracefully.
+ if (!getLangOpts().DeclSpecKeyword && Tok.is(tok::identifier) &&
+ Tok.getIdentifierInfo()->getName().equals("__declspec")) {
+// The next token should be an open paren. If it is, eat the entire
+// attribute declaration and continue.
+if (NextToken().is(tok::l_paren)) {
+ // Consume the __declspec identifier.
+ SourceLocation Loc = ConsumeToken();
+
+ // Eat the parens and everything between them.
+ BalancedDelimiterTracker T(*this, tok::l_paren);
+ if (T.consumeOpen()) {
+assert(false && "Not a left paren?");
+return;
+ }
+ T.skipToEnd();
+
+ Diag(Loc, diag::err_ms_attributes_not_enabled);
+ continue;
+}
+ }
+
// In C++, check to see if this is a scope specifier like foo::bar::, if
// so handle it as such. This is important for ctor parsing.
if (getLangOpts().CPlusPlus) {
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -176,6 +176,9 @@
def warn_attribute_no_decl : Warning<
"attribute %0 ignored, because it is not attached to a declaration">,
InGroup;
+def err_ms_attributes_not_enabled : Error<
+ "'__declspec' attributes are not enabled; use '-fdeclspec' or "
+ "'-fms-extensions' to enable support for __declspec attributes">;
def err_expected_method_body : Error<"expected method body">;
def err_declspec_after_virtspec : Error<
"'%0' qualifier may not appear after the virtual specifier '%1'">;
Index: test/Parser/declspec-supported.c
===
--- test/Parser/declspec-supported.c
+++ test/Parser/declspec-supported.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -fms-extensions -verify %s
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -fdeclspec -verify %s
+// expected-no-diagnostics
+
+__declspec(naked) void f(void) {}
+
+struct S {
+ __declspec(property(get=Getter, put=Setter)) int X;
+ int Y;
+};
Index: test/Parser/declspec-recovery.c
===
--- test/Parser/declspec-recovery.c
+++ test/Parser/declspec-recovery.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -verify %s
+
+__declspec(naked) void f(void) {} // expected-error{{'__declspec' attributes are not enabled; use '-fdeclspec' or '-fms-extensions' to enable support for __declspec attributes}}
+
+struct S {
+ __declspec(property(get=Getter, put=Setter)) int X; // expected-error{{'__declspec' attributes are not enabled; use '-fdeclspec' or '-fms-extensions' to enable support for __declspec attributes}}
+ int Y;
+};
Index: lib/Parse/ParseDecl.cpp
[PATCH] D28768: [clang-tidy] Add check 'modernize-return-braced-init-list'
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 be `unsigned` rather than `int`.
Comment at: clang-tidy/modernize/ReturnBracedInitListCheck.cpp:80-85
+ const SourceRange CallParensRange =
+ MatchedConstructExpr->getParenOrBraceRange();
+
+ // Return if there is no explicit constructor call.
+ if (CallParensRange.isInvalid())
+return;
This could be hoisted above the `for` loop to early return while doing less
work.
Comment at: clang-tidy/modernize/ReturnBracedInitListCheck.cpp:88
+ // Range for constructor name and opening brace.
+ auto CtorCallSourceRange = CharSourceRange::getTokenRange(
+ Loc, CallParensRange.getBegin().getLocWithOffset(-1));
Not keen on this use of `auto`, as the type is not spelled out in the
initialization.
Repository:
rL LLVM
https://reviews.llvm.org/D28768
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29868: Recover more gracefully when __declspec is not supported as a keyword
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 wrote:
> > > aaron.ballman wrote:
> > > > compnerd wrote:
> > > > > I think that we want to emit the diagnostic even if there is no
> > > > > parenthesis as `__declspec` is a reserved identifier, and we would
> > > > > normally diagnose the bad `__declspec` (expected '(' after
> > > > > '__declspec').
> > > > Yes, but it could also lead to a rejecting code that we used to accept
> > > > and properly handle when __declspec is an identifier rather than a
> > > > keyword. e.g.,
> > > > ```
> > > > struct __declspec {};
> > > >
> > > > __declspec some_func(void);
> > > > ```
> > > > By looking for the paren, we run less risk of breaking working code,
> > > > even if that code abuses the implementation namespace (after all,
> > > > __declspec it not a keyword in this scenario).
> > > But we would reject that code under `-fdeclspec` anyways. I think having
> > > the code be more portable is a bit nicer.
> > After discussing in IRC, I decided that I agree with @compnerd on this and
> > have changed the patch accordingly.
> What if somebody wants to use __declspec and are using the compiler in a
> freestanding mode? Also, we aren't the only member of the implementor's
> namespace.
Users using __declspec in a freestanding mode is a concern that I share.
However, I imagine there are *far* more users who accidentally forget to pass
`-fdeclspec` than there are users who are writing code in freestanding mode
that wish to use `__declspec` as an identifier in a situation that proves
problematic. That being said, do you think the approach in the patch would work
with a warning rather than an error? I went with an error because I felt that a
warning would require tentative parsing to be properly implemented, which felt
like a heavy solution for a problem I didn't think anyone would run into in
practice.
I'm not overly sympathetic to others in the implementor's namespace who use
`__declspec` but not to implement attributes under that name. However, I could
be convinced to be sympathetic if there was some conflict in practice. Do you
have a case in mind?
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] D29819: Introduce an 'external_source_symbol' attribute that describes the origin and the nature of a declaration
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", "external_source_symbol">];
+ let Args = [IdentifierArgument<"language", 1>,
Is this supported by GCC? If not, it should be under the clang namespace
instead of the gnu namespace. If it is supported by GCC, then you should just
use a single GCC spelling.
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
Would this hold something like a file name? If so, I worry about conflicts
between the comma separator and a file name -- you might want to pick a
separator that can't be used in a file name (like | or :).
Comment at: include/clang/Basic/AttrDocs.td:1015
+ This declaration was automatically generated by some tool.
+ }];
+}
Are these clauses parsed in a strict order? If so, you may want to mention that
the order matters (or doesn't).
Also, the code in SemaDeclAttr.cpp implies that some of these are optional. It
should be made clear which (if any) arguments are optional.
Comment at: include/clang/Basic/DiagnosticParseKinds.td:863
+def err_external_source_symbol_expected_language : Error<
+ "expected a source language , e.g., 'Swift'">;
+
I would drop the e.g. and instead describe what's really expected: an
identifier. The e.g. muddies the water because it suggests there's a list of
supported languages, and it shows something that looks kind of like a string
when it isn't one. Something like: `expected an identifier representing the
source language` or some such?
Comment at: lib/Parse/ParseDecl.cpp:1141
+} else {
+ // Keyword must be 'Ident_defined_in'.
+ if (Tok.isNot(tok::string_literal)) {
You may want to assert this.
Comment at: lib/Sema/SemaDeclAttr.cpp:2414
+ const AttributeList ) {
+ if (!checkAttributeAtLeastNumArgs(S, Attr, 1))
+return;
You should also diagnose if there's more than 3 arguments, no?
Comment at: lib/Sema/SemaDeclAttr.cpp:2418
+ if (!isa(D)) {
+S.Diag(Attr.getLoc(), diag::warn_attribute_ignored) << Attr.getName();
+return;
This isn't really the right diagnostic for a mismatched attribute subject. It
should be using `warn_attribute_wrong_decl_type` instead so that the user is
more clear on why the attribute is ignored.
Comment at: lib/Sema/SemaDeclAttr.cpp:2428
+DefinedIn = SE->getString();
+ bool IsGeneratedDeclaration = Attr.getArgAsIdent(2);
+
Rather than rely on the implicit conversion to bool, I think this is a case
where it should be tested against `nullptr` explicitly.
Comment at: test/Parser/attr-external-source-symbol-cxx11.cpp:6
+
+// expected-no-diagnostics
Please put this directly below the RUN line.
Comment at: test/Parser/attr-external-source-symbol.m:26
+void f6()
+__attribute__((external_source_symbol(defined_in=20))); // expected-error
{{expected string literal for source container name in 'external_source_symbol'
attribute}}
I think you can currently get away with writing
`external_source_symbol(generated_declaration, generated_declaration,
generated_declaration, defined_in="module"))` and other odd combinations.
There should be additional parser tests for totally crazy parsing scenarios.
You may want to consider running a fuzzer to generate some of them.
Comment at: test/Sema/attr-external-source-symbol.c:5
+
+void f2() __attribute__((external_source_symbol(generated_declaration)));
+
Should also have a sema test for when there are 2 args, and 4 args.
There should also be a test under Misc that also checks that the args are
properly lowered into the AST in the correct way with differing argument
orders. e.g, `external_source_symbol(generated_declaration, language=Swift,
defined_in="module")` vs `external_source_symbol(language=Swift,
generated_declaration, defined_in="module")` vs
`external_source_symbol(defined_in="module", language=Swift,
generated_declaration)`, etc.
Repository:
rL LLVM
https://reviews.llvm.org/D29819
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D23421: [Clang-tidy] CERT-MSC53-CPP (checker for std namespace modification)
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 there are times when it
is permissible to modify the std namespace outside of a system header. The
important bit of the CERT requirement is "Do not add declarations or
definitions to the standard namespaces std or posix, or to a namespace
contained therein, except for a template specialization that depends on a
user-defined type that meets the standard library requirements for the original
template." So the part that's missing from this check is excluding template
specializations that depend on user-defined types. For instance, the following
should be valid:
```
namespace std {
template <>
struct is_error_code_enum : std::true_type {};
}
```
(This comes from Error.h in LLVM.)
Comment at: clang-tidy/cert/DontModifyStdNamespaceCheck.cpp:36
+ "modification of '%0' namespace can result in undefined behavior")
+ << N->getName();
+}
No need to call `getName()` -- you can pass `N` directly. When you correct
this, also remove the explicit quotes from the diagnostic (they're added
automatically by the diagnostics engine).
Comment at: docs/clang-tidy/checks/cert-dcl58-cpp.rst:8
+behavior.
+This check warns for such modifications.
+
You should add a link to the CERT guideline this addresses. See
docs/clang-tidy/checks/cert-dcl50-cpp.rst as an example.
https://reviews.llvm.org/D23421
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29868: Recover more gracefully when __declspec is not supported as a keyword
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)
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 think this is missing function declarations that are explicit template
specializations as well (a common instance of this is specializing std::swap).
Comment at: test/clang-tidy/cert-dcl58-cpp.cpp:60
+}
+
+using namespace std;
I'd like to see a test where the user specializes a function template, like
`swap()` to make sure we don't diagnose on that.
https://reviews.llvm.org/D23421
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29839: [clang-tidy] New misc-istream-overflow check
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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29868: Recover more gracefully when __declspec is not supported as a keyword
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: test/Parser/declspec-supported.c
===
--- test/Parser/declspec-supported.c
+++ test/Parser/declspec-supported.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -fms-extensions
-verify %s
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -fdeclspec -verify %s
+// expected-no-diagnostics
+
+__declspec(naked) void f(void) {}
+
+struct S {
+ __declspec(property(get=Getter, put=Setter)) int X;
+ int Y;
+};
Index: test/Parser/declspec-recovery.c
===
--- test/Parser/declspec-recovery.c
+++ test/Parser/declspec-recovery.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -verify %s
+
+__declspec(naked) void f(void) {} // expected-error{{'__declspec' attributes
are not enabled; use '-fdeclspec' or '-fms-extensions' to enable support for
__declspec attributes}}
+
+struct S {
+ __declspec(property(get=Getter, put=Setter)) int X; //
expected-error{{'__declspec' attributes are not enabled; use '-fdeclspec' or
'-fms-extensions' to enable support for __declspec attributes}}
+ int Y;
+};
Index: lib/Parse/ParseDecl.cpp
===
--- lib/Parse/ParseDecl.cpp
+++ lib/Parse/ParseDecl.cpp
@@ -2966,6 +2966,31 @@
if (DS.hasTypeSpecifier())
goto DoneWithDeclSpec;
+ // If the token is an identifier named "__declspec" and Microsoft
+ // extensions are not enabled, it is likely that there will be cascading
+ // parse errors if this really is a __declspec attribute. Attempt to
+ // recognize that scenario and recover gracefully.
+ if (!getLangOpts().DeclSpecKeyword && Tok.is(tok::identifier) &&
+ Tok.getIdentifierInfo()->getName().equals("__declspec")) {
+Diag(Loc, diag::err_ms_attributes_not_enabled);
+
+// The next token should be an open paren. If it is, eat the entire
+// attribute declaration and continue.
+if (NextToken().is(tok::l_paren)) {
+ // Consume the __declspec identifier.
+ SourceLocation Loc = ConsumeToken();
+
+ // Eat the parens and everything between them.
+ BalancedDelimiterTracker T(*this, tok::l_paren);
+ if (T.consumeOpen()) {
+assert(false && "Not a left paren?");
+return;
+ }
+ T.skipToEnd();
+ continue;
+}
+ }
+
// In C++, check to see if this is a scope specifier like foo::bar::, if
// so handle it as such. This is important for ctor parsing.
if (getLangOpts().CPlusPlus) {
Index: include/clang/Basic/DiagnosticParseKinds.td
===
--- include/clang/Basic/DiagnosticParseKinds.td
+++ include/clang/Basic/DiagnosticParseKinds.td
@@ -176,6 +176,9 @@
def warn_attribute_no_decl : Warning<
"attribute %0 ignored, because it is not attached to a declaration">,
InGroup;
+def err_ms_attributes_not_enabled : Error<
+ "'__declspec' attributes are not enabled; use '-fdeclspec' or "
+ "'-fms-extensions' to enable support for __declspec attributes">;
def err_expected_method_body : Error<"expected method body">;
def err_declspec_after_virtspec : Error<
"'%0' qualifier may not appear after the virtual specifier '%1'">;
Index: test/Parser/declspec-supported.c
===
--- test/Parser/declspec-supported.c
+++ test/Parser/declspec-supported.c
@@ -0,0 +1,10 @@
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -fms-extensions -verify %s
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -fdeclspec -verify %s
+// expected-no-diagnostics
+
+__declspec(naked) void f(void) {}
+
+struct S {
+ __declspec(property(get=Getter, put=Setter)) int X;
+ int Y;
+};
Index: test/Parser/declspec-recovery.c
===
--- test/Parser/declspec-recovery.c
+++ test/Parser/declspec-recovery.c
@@ -0,0 +1,8 @@
+// RUN: %clang_cc1 -triple i386-pc-unknown -fsyntax-only -verify %s
+
+__declspec(naked) void f(void) {} // expected-error{{'__declspec' attributes are not enabled; use '-fdeclspec' or '-fms-extensions' to enable support for __declspec attributes}}
+
+struct S {
+ __declspec(property(get=Getter, put=Setter)) int X; // expected-error{{'__declspec' attributes are not enabled; use '-fdeclspec' or '-fms-extensions' to enable support for __declspec attributes}}
+ int Y;
+};
Index: lib/Parse/ParseDecl.cpp
===
---
[PATCH] D29868: Recover more gracefully when __declspec is not supported as a keyword
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 wrote:
> > > I think that we want to emit the diagnostic even if there is no
> > > parenthesis as `__declspec` is a reserved identifier, and we would
> > > normally diagnose the bad `__declspec` (expected '(' after '__declspec').
> > Yes, but it could also lead to a rejecting code that we used to accept and
> > properly handle when __declspec is an identifier rather than a keyword.
> > e.g.,
> > ```
> > struct __declspec {};
> >
> > __declspec some_func(void);
> > ```
> > By looking for the paren, we run less risk of breaking working code, even
> > if that code abuses the implementation namespace (after all, __declspec it
> > not a keyword in this scenario).
> But we would reject that code under `-fdeclspec` anyways. I think having the
> code be more portable is a bit nicer.
After discussing in IRC, I decided that I agree with @compnerd on this and have
changed the patch accordingly.
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] D28166: Properly merge K functions that have attributes
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
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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30009: Add support for '#pragma clang attribute'
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 relevant declaration individually, So then this code does not produce an error? ``` #pragma clang attribute push(hot) void func(void) __attribute__((cold)); #pragma clang attribute pop ``` Comment at: docs/LanguageExtensions.rst:2428 +whether or not an attribute is supported by the pragma by referring to the +:doc:`individual documentation for that attribute `. This doesn't make clear what would happen with something like: ``` #pragma clang attribute push(cdecl) void function(void (*fp)(int)); #pragma clang attribute pop ``` Naively, I would expect the cdecl attribute to appertain to both `function` and `fp`, but I'm guessing that's not what actually happens (because cdecl is both a decl and a type attribute at the same time). Also, why is this not available for __declspec attributes? Why do C++ attributes require the syntax to be present in the #pragma, but GNU-style attributes do not? There are also MS-style attribute (currently unsupported, but there have been some efforts to include them in the past) and __declspec attributes as well. Why not require the syntax so that any of them can be supported? 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 StmtAttr or a TypeAttr I have strong reservations about this -- users are going to have no idea what attributes are and are not supported because they're not going to know whether the attribute has a subject list or requires delayed parsing. We have a considerable number of attributes for which the Subjects line is currently commented out simply because no one has bothered to fix that. This means those attributes can't be used with this pragma until someone fixes that, but when it happens, they magically can be used, which is a good thing. But the converse is more problematic -- if there's an existing Subjects line that gets removed because a subject is added that is difficult to express in TableGen it may break user code. We can fix the discoverability issues somewhat by updating the documentation emitter to spit out some wording that says when an attribute is/is not supported by this feature, but that only works for attributes which have documentation and so it's not a particularly reliable workaround. Repository: rL LLVM https://reviews.llvm.org/D30009 ___ 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)
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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28768: [clang-tidy] Add check 'modernize-return-braced-init-list'
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 return type from the " +"declaration; use a braced initializer list instead"); This diagnostic reads strangely. I think you should drop the "to" at the start of the sentence. Repository: rL LLVM https://reviews.llvm.org/D28768 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28768: [clang-tidy] Add check 'modernize-return-braced-init-list'
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 the code. Why is
a braced init list better than another kind of initialization expression?
Perhaps: "to avoid repeating the return type from the declaration, use a braced
initializer list instead" or something along those lines?
Comment at: clang-tidy/modernize/ReturnBracedInitListCheck.cpp:62
+
+ auto CallParensRange = MatchedConstructExpr->getParenOrBraceRange();
+
Please do not use `auto` here, the type is not explicitly spelled out in the
initialization.
Comment at: test/clang-tidy/modernize-return-braced-init-list.cpp:38
+ return Foo(b);
+}
We should probably have a test that ensures this code
```
[]() { return std::vector({1, 2}); }();
```
does not get converted into this code
```
[]() { return {1, 2}; }();
```
Repository:
rL LLVM
https://reviews.llvm.org/D28768
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28850: [docs] Tell Doxygen to expand LLVM_ALIGNAS to nothing
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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28902: [Sema] Reword unused lambda capture warning
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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28924: [AST Printer] Print attributes on enum constants
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 {
One [[deprecated]]
};
Repository:
rL LLVM
https://reviews.llvm.org/D28924
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28924: [AST Printer] Print attributes on enum constants
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 that as well, or just land this > patch as is? This is a pervasive issue with attributes, now that I put my thinking cap on. I think you're fine to land it as-is; we need a much larger fix to handle pretty printing attributes properly for all flavors. Repository: rL LLVM https://reviews.llvm.org/D28924 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30327: [Sema] Improve side effect checking for unused-lambda-capture warning
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 LambdaExpr *LE = cast(this);
> for (LambdaExpr::capture_iterator I = LE->capture_begin(),
> E = LE->capture_end(); I != E; ++I)
> if (I->getCaptureKind() == LCK_ByCopy)
> // FIXME: Only has a side-effect if the variable is volatile or if
> // the copy would invoke a non-trivial copy constructor.
> return true;
> return false;
> }
>
>
> It seems a shame not to fix this now, but I don't think I can call Sema code
> from AST code, and my `CaptureHasSideEffects()` depends on
> `Sema::getCurrentThisType()`.
>
> Any ideas?
Correct, you cannot call Sema code from AST. I don't know of a good way to
resolve this off the top of my head given what `getCurrentThisType()` needs to
do. I agree that this would be good to fix as well, but your patch is a good
incremental improvement and should't be held up on this.
https://reviews.llvm.org/D30327
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28266: Transparent_union attribute should be possible in front of union (rework)
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)
{
+ for (const AttributeList* l = AttrList; l; l = l->getNext())
+if (l->getKind() == AttributeList::AT_TransparentUnion) {
aaron.ballman wrote:
> Nit: the `*` should bind to `l` and `l` should be `L` by our usual naming
> conventions (but a name better than `L` wouldn't be amiss).
Nit: missing a space before the first `=`. You should run clang-format on the
patch to catch this sort of thing.
https://reviews.llvm.org/D28266
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30166: Honor __unaligned in codegen for declarations and expressions
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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30327: [Sema] Improve side effect checking for unused-lambda-capture warning
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.
e.g., this should not diagnose either:
```
void f(volatile int v) {
[v]{}();
}
```
https://reviews.llvm.org/D30327
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28266: Transparent_union attribute should be possible in front of union (rework)
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 ProcessDeclAttributeDelayed(Decl *D, const AttributeList *AttrList);
I would remove the mention of TransparentUnion -- this is generalized for other
uses already.
Comment at: lib/Parse/ParseDeclCXX.cpp:1889
+ if (!TagOrTempResult.isInvalid())
+// Delayed proccessing TransparentUnion attribute.
+Actions.ProcessDeclAttributeDelayed(TagOrTempResult.get(),
attrs.getList());
I'd remove mention of TransparentUnion here as well.
Comment at: lib/Sema/SemaDecl.cpp:13499
+ if (Attr)
+ProcessDeclAttributeList(S, New, Attr);
+
erichkeane wrote:
> I wasn't sure if this was the correct change in here. My other option was to
> move the above 2 lines (13495/13496) above these line's original spot, but
> moving it here seems to have correctly passed all tests. Anyone with more
> knowledge here want to chime in?
I think this change is reasonable.
Comment at: lib/Sema/SemaDeclAttr.cpp:6201
+void Sema::ProcessDeclAttributeDelayed(Decl *D, const AttributeList *AttrList)
{
+ for (const AttributeList* l = AttrList; l; l = l->getNext())
+if (l->getKind() == AttributeList::AT_TransparentUnion) {
Nit: the `*` should bind to `l` and `l` should be `L` by our usual naming
conventions (but a name better than `L` wouldn't be amiss).
https://reviews.llvm.org/D28266
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.
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. This should probably be:
`'std::random_shuffle' has been removed in C++17; use 'std::shuffle' instead`.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:27
+
+static const StringRef RandomFunctionMessage =
+" The old user defined 'RandomFunction' is not usable for 'shuffle'. You "
I'd feel more comfortable if both of these were `const char[]` rather than
`StringRef`, because `StringRef` doesn't typically own the underlying memory
and should not be long-lived.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:28
+static const StringRef RandomFunctionMessage =
+" The old user defined 'RandomFunction' is not usable for 'shuffle'. You "
+"need to make additional changes if you want a specific random function.";
Instead of adding additional text in this case, would it make sense to use a
different diagnostic? e.g., `'std::random_shuffle' has been removed in C++17;
use an alternative mechanism instead` (or something along those lines).
Regardless, it shouldn't be using full sentences.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:71
+ const auto *MatchedCallExpr = Result.Nodes.getNodeAs("match");
+ SourceManager = *Result.SourceManager;
+ LangOptions LangOpts = getLangOpts();
No need for this to be a separate variable.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:72
+ SourceManager = *Result.SourceManager;
+ LangOptions LangOpts = getLangOpts();
+
No need for this to be a separate variable.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:77
+
+ auto Diag = [=]() {
+std::string Message = ReplaceMessage;
Is there a reason this needs to capture everything by copy? Also, no need for
the empty parens. Actually, is the lambda even necessary at all?
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:105
+
+ if (auto IncludeFixit = IncludeInserter->CreateIncludeInsertion(
+ Result.Context->getSourceManager().getFileID(
Please don't use `auto` as the type is not specified in the initialization.
Comment at: docs/clang-tidy/checks/modernize-replace-random-shuffle.rst:8
+
+Below is two examples of what kind of occurrences will be found and two
examples of what it will be replaced with.
+
is -> are
Comment at: docs/clang-tidy/checks/modernize-replace-random-shuffle.rst:20
+
+Both these examples will be replaced with
+
Both these -> Both of these
with -> with:
https://reviews.llvm.org/D30158
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30192: [Sema] Detecting more array index out of bounds
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 Stmt::CXXOperatorCallExprClass: {
+const CXXOperatorCallExpr *OCE = cast(expr);
+for (auto Arg : OCE->arguments())
You can use `const auto *` here.
Comment at: lib/Sema/SemaChecking.cpp:10614
+const CXXOperatorCallExpr *OCE = cast(expr);
+for (auto Arg : OCE->arguments())
+ CheckArrayAccess(Arg);
`const auto *` instead of just `auto`.
Repository:
rL LLVM
https://reviews.llvm.org/D30192
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D29819: Introduce an 'external_source_symbol' attribute that describes the origin and the nature of a declaration
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: lib/Parse/ParseDeclCXX.cpp:3818-3819
+ if (ScopeName && (ScopeName->getName() == "gnu" ||
+(ScopeName->getName() == "clang" &&
+ AttrName->isStr("external_source_symbol"
// GNU-scoped attributes have some special cases to handle GNU-specific
aaron.ballman wrote:
> I don't really like hard-coding a list of attributes like this. I think you
> should handle clang-namespaced attributes with a separate helper function.
This still isn't quite what I was looking for -- the helper function I was
talking about was a replacement for `ParseGNUAttributeArgs()`. I'm sorry if I
was unclear.
I think we should have a `ParseClangAttributeArgs()` that handles your custom
parsing; it can then call through to `ParseAttributeArgsCommon()` for common
argument handling in the same way `ParseGNUAttributeArgs()` does. So the
predicate would be `else if (ScopeName && ScopeName->getName() == "clang")`.
Repository:
rL LLVM
https://reviews.llvm.org/D29819
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D30166: Honor __unaligned in codegen for declarations and expressions
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 uncomfortable -- wouldn't we rather have the distinction between unqualified and qualified types instead of a mixture of both when calling `getTypeInfo()`? https://reviews.llvm.org/D30166 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26800: [Sema] Make __attribute__((notnull)) inheritable through function parameters
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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D14274: Add alloc_size attribute to clang
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, clang will only trace ``const`` pointers (as above); we give up + on pointers that are not marked as ``const``. In the vast majority of cases, + this is unimportant, because LLVM has support for the ``alloc_size`` Double space after the full stop should be a single space (here and below). https://reviews.llvm.org/D14274 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27815: [clang-tidy] Add obvious module for obvious bugs
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 is run less frequently. Since the checks are meant to be finding bugs rather than enforcing a coding standard, performance, readability, etc, a few possible alternative names: `bugs-` `correctness-` https://reviews.llvm.org/D27815 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27815: [clang-tidy] Add obvious module for obvious bugs
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, if the checks are > > finding *obvious bugs*, then that suggests they should be implemented in > > the clang frontend rather than a tool that is run less frequently. > > > > Since the checks are meant to be finding bugs rather than enforcing a > > coding standard, performance, readability, etc, a few possible alternative > > names: > > > > `bugs-` > > `correctness-` > > > My intention is to have kind of bugs that you facepalm when you find it. > Mostly simple typos. > As I said this can't make it to clang because it would have some false > positives. Many warnings in clang have some false positives. Do you envision these having a fairly high false positive rate, or expensive to check for? > I am ok with making group like `bugs` that would have some checks that are > curently in misc (misc-use-after-move), but still, this group would be > special in the sense that it looks for very simple bugs that you can make > during coding, but you won't be able to find it in code that seem to work. I'm having a hard time imagining how many checks would be in this module from that description. Are you expecting to move items from misc to populate this module (if so, which ones, aside from misc-use-after-move?), or are you intending to add some checks we don't currently have (if so, can you list a few of them)? From your description, it sounds like these are not obvious bugs (since the code seems to work and the bugs are hard to spot), but it may be that "bugs" or "correctness" could also be a poor name. With some examples of checks that could go in the module, we may be able to pick a more appropriate name. https://reviews.llvm.org/D27815 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D26453: [clang-tidy] Remove duplicated check from move-constructor-init
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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications
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;
Shouldn't we require C++11 or later?
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:64
+ Finder->addMatcher(
+ parmVarDecl(hasType(pointerType(pointee(parenType(innerType(
+ functionProtoType(hasDynamicExceptionSpec(
Will this catch pointers to member functions as well?
```
struct S {
void f() throw();
};
void f(void (S::*)() throw()) {
}
```
Comment at: clang-tidy/modernize/UseNoexceptCheck.cpp:68
+ this);
+}
+
Also missing: typedefs and using declarations.
Comment at: docs/clang-tidy/checks/modernize-use-noexcept.rst:30
+By default this check will only replace ``throw()`` with ``noexcept``,
+and ``throw([,...])`` or ``throw(...)`` with blank except
+for operator delete and destructors, which are replaced with
I continue to object to removing the explicit exception specification entirely
as the default behavior without strong justification. Further. there is no
option to control this behavior.
Comment at: docs/clang-tidy/checks/modernize-use-noexcept.rst:38
+that don't support the ``noexcept`` keyword. Users can define the
+macro to be ``noexcept`` or ``throw()`` depending on whether or not
+noexcept is supported. Specifications that can throw, e.g.,
I'm not certain I understand the justification of calling out older compilers
or suggesting use of `throw()`. The check will continually flag `throw()` and
try to recommend `noexcept` in its place, won't it? At least, that's how I read
the preceding paragraph.
I think the macro replacement is a reasonable feature, but I think the check
only makes sense for C++11 or later, since C++98 users really have no
alternatives to dynamic exception specifications.
https://reviews.llvm.org/D20693
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28260: Add an argumentsAre matcher
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: include/clang/ASTMatchers/ASTMatchers.h:3075
+///
+/// Example matches the call to f2, but not f1 or f3.
+/// (matcher = callExpr(argumentsAre(declRefExpr(), declRefExpr(
This is a neat matcher, but I'm not certain it will work with the dynamic
matchers, which is an unfortunate divergence for clang-query.
Another concern is that this is leaking implementation details into
ASTMatchers.h rather than keeping them in ASTMatchersInternal.h.
Comment at: unittests/ASTMatchers/ASTMatchersTraversalTest.cpp:383
+ EXPECT_TRUE(matches("void f(int x, int y) { f(x, y); }", Call));
+ EXPECT_TRUE(notMatches("void f(int x, int y, int z) { f(x, y, z); }", Call));
+}
How does this matcher work in the presence of default arguments? e.g.,
```
void f(int a, int b = 12);
f(1);
```
Will `callExpr(argumentsAre(integerLiteral()))` match?
A similar question applies for variadic functions and functions without a
prototype (from C).
I suspect it all works fine, but some test cases would be nice.
https://reviews.llvm.org/D28260
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28260: Add an argumentsAre matcher
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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28451: [AVR] Add support for the 'interrupt' and 'naked' attributes
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, arguments are present, etc.
Comment at: include/clang/Basic/Attr.td:485
+ let ParseKind = "Interrupt";
+ let Documentation = [Undocumented];
+}
No new undocumented attributes, please.
Comment at: include/clang/Basic/Attr.td:488
+
+def AVRSignal : InheritableAttr, TargetSpecificAttr {
+ let Spellings = [GNU<"signal">];
Does this attribute appertain to any specific subjects, or can you apply it to
any declaration?
Comment at: include/clang/Basic/Attr.td:490
+ let Spellings = [GNU<"signal">];
+ let Documentation = [Undocumented];
+}
No new undocumented attributes, please.
Comment at: test/CodeGen/avr/attributes/naked.c:4
+// CHECK: define void @foo() #0
+__attribute__((naked)) void foo(void) { }
+
This test seems unrelated as you didn't modify anything about the naked
attribute?
https://reviews.llvm.org/D28451
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D20428: Tracking exception specification source locations
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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28467: [Sema] Add warning for unused lambda captures
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
>
>
> /home/buildbots/ppc64be-sanitizer/sanitizer-ppc64be/build/llvm/lib/Analysis/ValueTracking.cpp:1116:17:
> error: lambda capture 'BitWidth' is not used
> [-Werror,-Wunused-lambda-capture]
> auto KOF = [BitWidth](const APInt , unsigned ShiftAmt) {
> ^
>
> /home/buildbots/ppc64be-sanitizer/sanitizer-ppc64be/build/llvm/lib/Analysis/ValueTracking.cpp:1127:17:
> error: lambda capture 'BitWidth' is not used
> [-Werror,-Wunused-lambda-capture]
> auto KZF = [BitWidth](const APInt , unsigned ShiftAmt) {
> ^
>
> /home/buildbots/ppc64be-sanitizer/sanitizer-ppc64be/build/llvm/lib/Analysis/ValueTracking.cpp:1131:17:
> error: lambda capture 'BitWidth' is not used
> [-Werror,-Wunused-lambda-capture]
> auto KOF = [BitWidth](const APInt , unsigned ShiftAmt) {
> ^
> 3 errors generated.
>
>
> The warnings are correct.
>
> Should I remove the unused lambda captures or revert this changeset?
The warnings look correct to me; I think you should remove the unused lambda
captures (no need for a test or review since changes are NFC).
Repository:
rL LLVM
https://reviews.llvm.org/D28467
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28703: [clang] Emit `diagnose_if` warnings from system headers
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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28729: [clang-tidy] Add -enable-alpha-checks command
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 entirely blank. It makes it easier on the reviewers if they have an idea of what to expect and why it's needed. ;-) Also, this should probably be added to the release notes. Comment at: clang-tidy/ClangTidyOptions.h:78 + /// \brief Turns on experimental alpha checkers from static-analyzer. + llvm::Optional EnableAlphaChecks; from static-analyzer -> from the static analyzer https://reviews.llvm.org/D28729 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28672: [ASTMatchers] update doc by running dump_ast_matchers.py
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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28520: Disable -Wthread-safety-analysis for some functions in __thread_support
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? https://reviews.llvm.org/D28520 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28467: [Sema] Add warning for unused lambda captures
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}}
+ auto explicit_by_value_unused_decltype = [i] { decltype(i) j = 0; }; //
expected-warning{{lambda capture 'i' is not used}}
+
malcolm.parsons wrote:
> aaron.ballman wrote:
> > malcolm.parsons wrote:
> > > I don't know why the unevaluated context warning didn't work here.
> > Hmm, it should have, come to think of it. I don't think that's an odr-use
> > (when I get near a standard, I'll double-check), and it is unevaluated.
> > Worth poking at.
> The unevaluated context warning worked on line 38 where it's not in a
> template and on line 85 where it's `sizeof` instead of `decltype`.
>
> Somehow the template instantiation is avoiding some work when the capture
> doesn't have a dependent type.
That might make some degree of sense. Btw, I did double check, and it's
definitely not an odr-use.
I think this edge case can be handled in a follow-up patch if you want to get
the primary work in. If you'd prefer to have it fixed for the initial commit,
that's obviously fine too.
https://reviews.llvm.org/D28467
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28467: [Sema] Add warning for unused lambda captures
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 other capturing information in the future I would
> say that this should be its own field, but I'm not aware of anything else
> that's needed for this class. So I guess it would be better to pack it into
> `VarAndNestedAndThis`, but I wouldn't say it's a deal breaker.
I'm not keen on inconsistently initializating data members; can you perform
this initialization in the constructor instead?
Comment at: lib/Parse/ParseExprCXX.cpp:738
/// \return A DiagnosticID if it hit something unexpected. The location for
-/// for the diagnostic is that of the current token.
+/// the diagnostic is that of the current token.
Optional Parser::ParseLambdaIntroducer(LambdaIntroducer ,
This change is unrelated to the patch; you can go ahead and commit it without
review rather than have it as part of this patch.
Comment at: lib/Sema/SemaLambda.cpp:1479
for (unsigned I = 0, N = LSI->Captures.size(); I != N; ++I, ++CurField) {
- LambdaScopeInfo::Capture From = LSI->Captures[I];
+ LambdaScopeInfo::Capture = LSI->Captures[I];
assert(!From.isBlockCapture() && "Cannot capture __block variables");
Why does `From` need to be a non-const reference?
Comment at: lib/Sema/SemaLambda.cpp:1483
+ // Warn about unused explicit captures
+ if (!CurContext->isDependentContext() && !IsImplicit && !From.isUsed()) {
Missing a full stop at the end of the comment.
Comment at: test/SemaCXX/warn-unused-lambda-capture.cpp:17
+ auto explicit_by_value_unused = [i] {}; // expected-warning{{lambda capture
'i' is not used}}
+ auto explicit_by_value_unused_sizeof = [i] { return sizeof(i); }; //
expected-warning{{lambda capture 'i' is not used}}
+
This does not match the behavior for other -Wunused flags. e.g.,
```
void f() {
int i;
(void)sizeof(i);
}
```
I don't think diagnosing this test case is correct.
Comment at: test/SemaCXX/warn-unused-lambda-capture.cpp:59
+ auto explicit_by_value_unused = [i] {}; // expected-warning{{lambda capture
'i' is not used}}
+ auto explicit_by_value_unused_sizeof = [i] { return sizeof(i); }; //
expected-warning{{lambda capture 'i' is not used}}
+
Likewise here.
https://reviews.llvm.org/D28467
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications
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 macro. hintonda wrote: > alexfh wrote: > > alexfh wrote: > > > This description doesn't say why `noexcept` is better. > > This still needs to be addressed. > Absolutely, but I'll probably do all the code changes first while it's still > fresh in my mind. Since we're talking about documentation, should we note that this check does produce a subtle, obscure change in behavior? When you have `throw()` on a function signature and that function winds up throwing something, this would result in `std::unexpected()` being called. When that gets modified to be `noexcept`, the call to `std::unexpected()` is replaced by a call to `std::terminate()`. This hopefully does not break any code, but we may want to document the fact that failing to honor a dynamic exception specification is slightly different than failing to honor an exception specification so that users are aware of the slightly different semantics. https://reviews.llvm.org/D20693 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28467: [Sema] Add warning for unused lambda captures
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 explicit_initialized_value_used = [j = 1] { return j + 1; };
+ auto explicit_initialized_value_unused = [j = 1] {}; //
expected-warning{{lambda capture 'j' is not used}}
+
malcolm.parsons wrote:
> aaron.ballman wrote:
> > malcolm.parsons wrote:
> > > Quuxplusone wrote:
> > > > Would this still produce a diagnostic if `decltype(j)` were something
> > > > complicated like `lock_guard` or `string`? Presumably it should do the
> > > > same thing, more or less, as the other -Wunused diagnostics, which I
> > > > think is "don't warn if the constructor/destructor might actually do
> > > > important work".
> > > I was planning to check for that; thanks for the reminder.
> > One more complex case:
> > ```
> > #include
> >
> > struct B {
> > virtual ~B() = default;
> > };
> >
> > struct D : B {};
> >
> > struct C {
> > B& get_d() const;
> > C get_c() const;
> > };
> >
> > void f() {
> > C c1, c2;
> > [c1, c2] {
> > (void)typeid(c1.get_d());
> > (void)typeid(c2.get_c());
> > }();
> > }
> > ```
> > Hopefully this does not diagnose `c1` as being unused but still diagnoses
> > `c2` as unused.
> These tests are run with `-nostdsysteminc`, so I ran it manually:
> ```
> aaron.cpp:17:21: warning: expression with side effects will be evaluated
> despite being used as an operand to 'typeid'
> [-Wpotentially-evaluated-expression]
> (void)typeid(c1.get_d());
> ^
> aaron.cpp:16:8: warning: lambda capture 'c2' is not required to be captured
> for use in an unevaluated context [-Wunused-lambda-capture]
> [c1, c2] {
>^
> 2 warnings generated.
> ```
Perfect!
https://reviews.llvm.org/D28467
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28451: [AVR] Add support for the 'interrupt' and 'naked' attributes
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 specific subjects, or can you apply it
> > to any declaration?
> It can be applied to function definitions only.
Then it should have a Subjects line like `AVRInterrupt` does.
Comment at: lib/CodeGen/TargetInfo.cpp:6898
+ CodeGen::CodeGenModule ) const override {
+const FunctionDecl *FD = dyn_cast_or_null(D);
+if (!FD) return;
You can use `const auto *` here.
Comment at: lib/Sema/SemaDeclAttr.cpp:5129
+static void handleAVRInterruptAttr(Sema , Decl *D,
+ const AttributeList ) {
You can remove this function.
Comment at: lib/Sema/SemaDeclAttr.cpp:5136
+
+static void handleAVRSignalAttr(Sema , Decl *D,
+const AttributeList ) {
...and this one.
Comment at: lib/Sema/SemaDeclAttr.cpp:5158
+ case llvm::Triple::avr:
+handleAVRInterruptAttr(S, D, Attr);
+break;
Just call `handleSimpleAttribute()` instead.
Comment at: lib/Sema/SemaDeclAttr.cpp:5721
+ case AttributeList::AT_AVRSignal:
+handleAVRSignalAttr(S, D, Attr);
+break;
...same here.
Comment at: test/CodeGen/avr/attributes/naked.c:4
+// CHECK: define void @foo() #0
+__attribute__((naked)) void foo(void) { }
+
dylanmckay wrote:
> aaron.ballman wrote:
> > This test seems unrelated as you didn't modify anything about the naked
> > attribute?
> I didn't modify the naked attribute, but I did want to include a test for AVR.
This should probably be a separate commit then (and doesn't really need code
review, since it's just adding a test and is otherwise NFC); just make sure the
commit message explains why the test is beneficial to add.
https://reviews.llvm.org/D28451
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D20428: Tracking exception specification source locations
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. https://reviews.llvm.org/D20428 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28467: [Sema] Add warning for unused lambda captures
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" in any of our other frontend diagnostics; I
don't think this is a term that users will actually understand. I think we
should drop the term and just use "used".
Comment at: include/clang/Sema/ScopeInfo.h:457
+/// lambda.
+bool Used = false;
+
malcolm.parsons wrote:
> aaron.ballman wrote:
> > arphaman wrote:
> > > malcolm.parsons wrote:
> > > > Should this be moved into one of the `PointerIntPair`s?
> > > I'm not sure.. If we needed other capturing information in the future I
> > > would say that this should be its own field, but I'm not aware of
> > > anything else that's needed for this class. So I guess it would be better
> > > to pack it into `VarAndNestedAndThis`, but I wouldn't say it's a deal
> > > breaker.
> > I'm not keen on inconsistently initializating data members; can you perform
> > this initialization in the constructor instead?
> I'm not keen on repeating the initialization in every constructor.
Consistency is the important bit (see the golden rule in
http://llvm.org/docs/CodingStandards.html) -- if you want to move things to be
in-class initializers, that would be a reasonable follow-up patch. For this
patch, please put the initializer into the constructor.
Comment at: lib/Sema/SemaLambda.cpp:1479
for (unsigned I = 0, N = LSI->Captures.size(); I != N; ++I, ++CurField) {
- LambdaScopeInfo::Capture From = LSI->Captures[I];
+ LambdaScopeInfo::Capture = LSI->Captures[I];
assert(!From.isBlockCapture() && "Cannot capture __block variables");
malcolm.parsons wrote:
> aaron.ballman wrote:
> > Why does `From` need to be a non-const reference?
> It's not related to the warning; it looks like an unnecessary copy.
Thanks for adding the `const` qualifier.
Comment at: test/SemaCXX/warn-unused-lambda-capture.cpp:17
+ auto explicit_by_value_unused = [i] {}; // expected-warning{{lambda capture
'i' is not used}}
+ auto explicit_by_value_unused_sizeof = [i] { return sizeof(i); }; //
expected-warning{{lambda capture 'i' is not used}}
+
malcolm.parsons wrote:
> Quuxplusone wrote:
> > malcolm.parsons wrote:
> > > Quuxplusone wrote:
> > > > aaron.ballman wrote:
> > > > > This does not match the behavior for other -Wunused flags. e.g.,
> > > > > ```
> > > > > void f() {
> > > > > int i;
> > > > > (void)sizeof(i);
> > > > > }
> > > > > ```
> > > > > I don't think diagnosing this test case is correct.
> > > > I see some value to maybe diagnosing *something* here. For example, `[]
> > > > { return sizeof(i); }` would produce the same result as `[i] { return
> > > > sizeof(i); }` but with one fewer capture, so removing the `i` might be
> > > > seen as an improvement.
> > > >
> > > > But I'm not sure how to convey this information to the user. You could
> > > > say "variable `i` used in unevaluated context refers to the `i` in the
> > > > outer scope, not the captured `i`"... except that I think that would be
> > > > false. Given that you *have* captured an `i`, `sizeof(i)` definitely
> > > > refers to the captured one AFAIK. The fact that the captured `i`
> > > > shadows an `i` in the outer scope is irrelevant --- in fact the user is
> > > > *expecting* to shadow the outer `i`.
> > > >
> > > > Perhaps it would be appropriate to reword the diagnostic to "lambda
> > > > captures variable `i` unnecessarily". I would also lean toward
> > > > splitting it into two diagnostics — one for "this capture is
> > > > unnecessary" (as in this example) and one for "this capture doesn't
> > > > even appear lexically in the body of the lambda". Not sure how other
> > > > people would feel about that.
> > > >
> > > > You should add some test cases with `decltype(i)` for the same reason
> > > > as `sizeof(i)`.
> > > Does "lambda capture 'i' is not odr-used" make more sense?
> > > Does "lambda capture 'i' is not odr-used" make more sense?
> >
> > That would completely satisfy *me*, for what that's worth. It admittedly
> > doesn't match the other -Wunused diagnostics, but it is concise and correct
> > — at least I assume it's correct. :)
> C++14 [expr.prim.lambda]p18:
>
> > [ Note: An id-expression that is not an odr-use refers to the original
> > entity, never to a member
> > of the closure type. Furthermore, such an id-expression does not cause the
> > implicit capture of the entity.
> > — end note ]
Yes, that is correct; it's just incredibly confusing to most mortal
programmers. Perhaps instead we can add extra information to the diagnostic
telling the user that the variable need not be captured when it's in an
unevaluated context (if possible). e.g.,
"lambda
[PATCH] D20428: Tracking exception specification source locations
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 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28467: [Sema] Add warning for unused lambda captures
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}}
+ auto explicit_by_value_unused_decltype = [i] { decltype(i) j = 0; }; //
expected-warning{{lambda capture 'i' is not used}}
+
malcolm.parsons wrote:
> I don't know why the unevaluated context warning didn't work here.
Hmm, it should have, come to think of it. I don't think that's an odr-use (when
I get near a standard, I'll double-check), and it is unevaluated. Worth poking
at.
https://reviews.llvm.org/D28467
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28729: [clang-tidy] Add -enable-alpha-checks command
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 with the Static Analyzer maintainers, alpha checkers are > > > completely unsupported and are suitable for very early testing only. We > > > had problems with them routinely, that's why I disabled alpha checkers in > > > clang-tidy completely. I don't think there should be a user-visible way > > > to enable them. Developers can locally change the code to get access to > > > alpha checkers, but released binaries shouldn't provide this possibility. > > > > > > That's good to know -- should it be documented a bit more explicitly, > > > Yes, a comment to that effect near the relevant code wouldn't hurt. Or do you > have other suggestions? A comment in the code would be okay, but I think that removing all public mention of the alpha checkers (help text and website) would also be useful; I would not have thought that a production compiler would carry these checks for this many years if they weren't stable and useful. > > >> or perhaps the alpha checks should be removed until they're fit for public >> consumption? Some of those alpha checks have been in the product for a long >> time, and if they're so unstable that we cannot expose them in a >> user-friendly fashion, perhaps they don't belong yet? > > As discussed with SA folks, alpha checkers are convenient for them to develop > new checks, but shouldn't be exposed to users. Some of these experimental > checkers might deserve being moved out of alpha or removed completely, but > that should be reported to and discussed with the SA maintainers. Agreed. https://reviews.llvm.org/D28729 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D20428: Tracking exception specification source locations
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: > Richard, is it okay to commit this? If Richard does not respond today, then yes, it's fine to commit (it's had relatively extensive review already, and we can correct any outstanding issues post-commit). https://reviews.llvm.org/D20428 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28520: Disable -Wthread-safety-analysis for some functions in __thread_support
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 cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications
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 specifications to > typedefs and using declarations. Hmm, I thought you could, at least in C++17, since it's now part of the function's type. However, I don't have a standard in front of me at the moment, so I'll have to double-check. It can always be added in a follow-up patch and need not block this one. However, I do know the following is allowed in a typedef, and I don't think your existing ParmVarDecl matcher will catch it: ``` typedef void (*fp)(void (*f)(int) throw()); ``` Comment at: docs/clang-tidy/checks/modernize-use-noexcept.rst:30 +By default this check will only replace ``throw()`` with ``noexcept``, +and ``throw([,...])`` or ``throw(...)`` with blank except +for operator delete and destructors, which are replaced with hintonda wrote: > aaron.ballman wrote: > > I continue to object to removing the explicit exception specification > > entirely as the default behavior without strong justification. Further. > > there is no option to control this behavior. > I tried to make sure it's only applied where appropriate. If I missed a > case, please let me know, but I'm not sure an option "just in case" is right > solution. > > However, I did try to structure the code in a way to make it easy to add an > option if that turns out the be the right thing to do. The behavior as it's implemented now is not the behavior I would expect from such a check -- I think that `throw(int)` should get a FixIt to `noexcept(false)` rather than no `noexcept` clause whatsoever. Despite them being functionally equivalent in most cases, the explicit nature of the dynamic exception specification should be retained with an explicit noexcept exception specification, not discarded. If you really want `throw(int)` to be modified to have no explicit exception specification, I think that should be an option (off by default). If you would rather not make an option, then I think the explicit exception specification should remain. Comment at: docs/clang-tidy/checks/modernize-use-noexcept.rst:38 +that don't support the ``noexcept`` keyword. Users can define the +macro to be ``noexcept`` or ``throw()`` depending on whether or not +noexcept is supported. Specifications that can throw, e.g., hintonda wrote: > aaron.ballman wrote: > > I'm not certain I understand the justification of calling out older > > compilers or suggesting use of `throw()`. The check will continually flag > > `throw()` and try to recommend `noexcept` in its place, won't it? At least, > > that's how I read the preceding paragraph. > > > > I think the macro replacement is a reasonable feature, but I think the > > check only makes sense for C++11 or later, since C++98 users really have no > > alternatives to dynamic exception specifications. > Libraries, e.g., libc++, that need to support both multiple versions of the > standard, use macros to switch between throw() and noexcept. > > So this option was designed to be libc++ friendly. If that's not > appropriate, I can remove it. I think the *option* is fine; I think the wording is confusing. How about: ``` Alternatively, users can also use :option:`ReplacementString` to specify a macro to use instead of ``noexcept``. This is useful when maintaining source code that uses custom exception specification marking other than ``noexcept``. ``` https://reviews.llvm.org/D20693 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28520: Disable -Wthread-safety-analysis for some functions in __thread_support
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 existence of these attributes. I'll take a look. This may be of some help: http://clang.llvm.org/docs/ThreadSafetyAnalysis.html https://reviews.llvm.org/D28520 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27621: [clang-tidy] check to find declarations declaring more than one name
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:
> aaron.ballman wrote:
> > firolino wrote:
> > > aaron.ballman wrote:
> > > > firolino wrote:
> > > > > aaron.ballman wrote:
> > > > > > firolino wrote:
> > > > > > > firolino wrote:
> > > > > > > > firolino wrote:
> > > > > > > > > firolino wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > firolino wrote:
> > > > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > > > Why do we not want to match this?
> > > > > > > > > > > > If we decide, whether we transform
> > > > > > > > > > > > ```
> > > > > > > > > > > > class A {
> > > > > > > > > > > > } Object1, Object2;
> > > > > > > > > > > > ```
> > > > > > > > > > > > to
> > > > > > > > > > > > ```
> > > > > > > > > > > > class A {
> > > > > > > > > > > > } Object1,
> > > > > > > > > > > > Object2;
> > > > > > > > > > > > ```
> > > > > > > > > > > > or
> > > > > > > > > > > > ```
> > > > > > > > > > > > class A {
> > > > > > > > > > > > }
> > > > > > > > > > > > Object1,
> > > > > > > > > > > > Object2;
> > > > > > > > > > > > ```
> > > > > > > > > > > > I might consider adding support for it. Moreover, this
> > > > > > > > > > > > kind of definition is usually seen globally and I don't
> > > > > > > > > > > > know how to handle globals yet. See
> > > > > > > > > > > > http://lists.llvm.org/pipermail/cfe-dev/2015-November/046262.html
> > > > > > > > > > > I think this should be handled. It can be handled in
> > > > > > > > > > > either of the forms you show, or by saying:
> > > > > > > > > > > ```
> > > > > > > > > > > A Object1;
> > > > > > > > > > > A Object2;
> > > > > > > > > > > ```
> > > > > > > > > > > If all of these turn out to be a problem, we can still
> > > > > > > > > > > diagnose without providing a fixit.
> > > > > > > > > > >
> > > > > > > > > > > As for globals in general, they can be handled in a
> > > > > > > > > > > separate patch once we figure out the declaration
> > > > > > > > > > > grouping.
> > > > > > > > > > OK. I will try to split the object definition from the
> > > > > > > > > > class definition, as you have suggested. Thus, I can kick
> > > > > > > > > > out the tagDecl-matcher as well. If there is no easy way to
> > > > > > > > > > do this, it will be reported anyway but without a fixit.
> > > > > > > > > >
> > > > > > > > > > Note for me: Update documentation!
> > > > > > > > > What about
> > > > > > > > > ```
> > > > > > > > > struct S {
> > > > > > > > > } S1;
> > > > > > > > > ```
> > > > > > > > > I would like to report this too, since two names are being
> > > > > > > > > declared here. `S` and `S1`. What do you think?
> > > > > > > > ```
> > > > > > > > struct {
> > > > > > > > } nn1, nn2;
> > > > > > > > ```
> > > > > > > > Shall we ignore anonymous definitions?
> > > > > > > To be more precise: Warn and provide a fixit for `struct S {}
> > > > > > > S1`. Only warn for `struct {} nn1, nn2`.
> > > > > > > What about
> > > > > > >
> > > > > > > ```
> > > > > > > struct S {
> > > > > > > } S1;
> > > > > > > ```
> > > > > > > I would like to report this too, since two names are being
> > > > > > > declared here. S and S1. What do you think?
> > > > > >
> > > > > > I don't think that this should be diagnosed. For one, this is a
> > > > > > relatively common pattern (arguably more common than `struct S { }
> > > > > > S1, S2;`), but also, it names two very distinct entities (a type
> > > > > > and a variable).
> > > > > >
> > > > > > > ```
> > > > > > > struct {
> > > > > > > } nn1, nn2;
> > > > > > >```
> > > > > > > Shall we ignore anonymous definitions?
> > > > > >
> > > > > > Yes, we basically have to.
> > > > > >
> > > > > Transforming
> > > > > ```
> > > > > typedef struct X { int t; } X, Y;
> > > > > ```
> > > > > to
> > > > > ```
> > > > > typedef struct X { int t; };
> > > > > typedef X X;
> > > > > typedef X Y;
> > > > > ```
> > > > > will be valid, but looks odd.
> > > > I am on the fence about this transformation -- it does not declare new
> > > > objects, but instead declares new types. A very common pattern in some
> > > > libraries (such as the Win32 SDK) is to declare:
> > > > ```
> > > > typedef struct Blah {
> > > >
> > > > } Blah, *PBlah, **PPBlah;
> > > > ```
> > > > Because of that pattern, diagnosing the above typedef is likely to be
> > > > very chatty in some projects, and I don't know that the fixit provides
> > > > much real benefit for types.
> > > >
> > > > At least for the CERT DCL04-C version of this rule, that code should
> > > > not be flagged. Do you think any of the variations of this check should
> > > > flag it?
> > > Maybe we should reconsider where to split and where not. The original
> > > idea was to minimize confusion on code like
[PATCH] D27424: Add the diagnose_if attribute to clang.
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 clear, I'm happy to use it in
> loops, too; I'd just like to know if we have guidelines about it. :) )
We do:
http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
Btw, there's a question in there about late parsing that Phab won't let me
delete, feel free to ignore it. ;-)
Comment at: include/clang/Basic/Attr.td:1584
+def DiagnoseIf : InheritableAttr {
+ let Spellings = [GNU<"diagnose_if">];
+ let Subjects = SubjectList<[Function]>;
george.burgess.iv wrote:
> aaron.ballman wrote:
> > I think this should also have a C++ spelling under the clang namespace.
> Agreed. Though, it looks like that would requires us teaching the parser how
> to late-parse c++11 attrs if we wanted to do it properly. Given the size of
> this patch, I think that can be done as a follow-up.
That seems reasonable to me.
Comment at: include/clang/Sema/Sema.h:2616
+ DiagnoseIfAttr *
+ checkArgDependentDiagnoseIf(FunctionDecl *Function, ArrayRef Args,
+ SmallVectorImpl ,
george.burgess.iv wrote:
> aaron.ballman wrote:
> > Can this (and the other new functions) accept the `FunctionDecl` as a
> > `const FunctionDecl *` instead?
> Not easily, unless you'd prefer to see `const` here and a `const_cast` or two
> in `checkArgDependentDiagnoseIf` over what we have now. I have no preference,
> so if you would prefer to see that, I'm happy to do it.
>
> If `ExprResult` and initialization routines like
> `Sema::PerformCopyInitialization` were happy to take a `const Expr *`, this
> would be a different story :)
That sounds like more churn than it's worth, to me. It'd be nice if we were
better about const-correctness, but that's not this patch's problem to solve.
;-)
Comment at: lib/AST/ExprConstant.cpp:10371
+assert(MD && "Don't provide `this` for non-methods.");
+assert(!MD->isStatic() && "Don't provide `this` for non-static methods.");
+#endif
I think the message means to say "Don't provide 'this' for static methods."
Comment at: lib/Parse/ParseDecl.cpp:371-372
return;
}
// These may refer to the function arguments, but need to be parsed early to
In the last patch, this code was checking for `enable_if` or `diagnose_if`, but
now only checks for `enable_if`. Do you not need this for `diagnose_if` because
it's now late parsed?
Comment at: lib/Sema/SemaDeclAttr.cpp:949
+#ifndef NDEBUG
+if (auto *MD = dyn_cast(FD))
+ ClassType = MD->getParent();
`const auto *`
https://reviews.llvm.org/D27424
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications
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 as having no noexcept > specifier at all. > And less code to read means its easier to understand. If the API designer explicitly specified "this function can throw A or B", I think a more helpful FixIt is to specify `noexcept(false)` explicitly rather than leave off any information about the exception specification. The explicit nature of the dynamic exception specification suggests the API designer intended for the user to know more information about whether the function throws or not, so it's a bit hostile to replace that with no information about whether the function throws or not (even if it's functionally equivalent). https://reviews.llvm.org/D20693 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28180: Fixing small errors in libAST Matcher Tutorial.
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 http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28166: Properly merge K functions that have attributes
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 reasonable on its own?
> > Given that it turns working code into UB, I think my preference is to gate
> > it on a fix for the lowering bug, but I'm also not certain I am the right
> > person to implement that fix (though I could give it a shot).
>
>
> The test in question has a comment pointing to PR7117, which in turn
> indicates that we might miscompile parts of FreeBSD if we land this as-is. So
> I think we need to gate this on a fix for the lowering bug.
I agree with that logic, but would also point out that this bug appears to have
nothing to do with attributed functions, but K calls in general. Removing the
`__stdcall` entirely shows an identical issue:
void g(int n) {}
void (*p)() = g;
void f() { p(0);}
generates
; ModuleID = '-'
source_filename = "-"
target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-pc-windows-msvc"
@p = global void (...)* bitcast (void (i32)* @g to void (...)*), align 8
; Function Attrs: noinline nounwind
define void @g(i32 %n) #0 {
entry:
%n.addr = alloca i32, align 4
store i32 %n, i32* %n.addr, align 4
ret void
}
; Function Attrs: noinline nounwind
define void @f() #0 {
entry:
%0 = load void (...)*, void (...)** @p, align 8
%callee.knr.cast = bitcast void (...)* %0 to void (i64)*
call void %callee.knr.cast(i64 0)
ret void
}
attributes #0 = { noinline nounwind
"correctly-rounded-divide-sqrt-fp-math"="fal
se" "disable-tail-calls"="false" "less-precise-fpmad"="false"
"no-frame-pointer-
elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false"
"no-nans-fp-mat
h"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false"
"stack-p
rotector-buffer-size"="8" "target-features"="+mmx,+sse,+sse2,+x87"
"unsafe-fp-ma
th"="false" "use-soft-float"="false" }
!llvm.ident = !{!0}
!0 = !{!"clang version 4.0.0 (trunk 290670)"}
I'm happy to try to work on fixing this, but am unfamiliar with this part of
the codegen. I *think* the problem is that we gin up the function type for a
non-prototyped function based on the function call expression argument types,
and the literal `0` is getting the type `signed long long`. This forces us to
bitcast the function to one taking an i64 rather than an i32. If you switch the
argument from the literal `0` to a variable of type `int`, the bitcast becomes
`%3 = bitcast void (...)* %1 to void (i32)*`, which is correct (at least, as I
interpret the comments left in `EmitCall()` around line 4245).
Suggestions and hints welcome. :-)
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] D27621: [clang-tidy] check to find declarations declaring more than one name
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:
> aaron.ballman wrote:
> > firolino wrote:
> > > firolino wrote:
> > > > firolino wrote:
> > > > > firolino wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > firolino wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > Why do we not want to match this?
> > > > > > > > If we decide, whether we transform
> > > > > > > > ```
> > > > > > > > class A {
> > > > > > > > } Object1, Object2;
> > > > > > > > ```
> > > > > > > > to
> > > > > > > > ```
> > > > > > > > class A {
> > > > > > > > } Object1,
> > > > > > > > Object2;
> > > > > > > > ```
> > > > > > > > or
> > > > > > > > ```
> > > > > > > > class A {
> > > > > > > > }
> > > > > > > > Object1,
> > > > > > > > Object2;
> > > > > > > > ```
> > > > > > > > I might consider adding support for it. Moreover, this kind of
> > > > > > > > definition is usually seen globally and I don't know how to
> > > > > > > > handle globals yet. See
> > > > > > > > http://lists.llvm.org/pipermail/cfe-dev/2015-November/046262.html
> > > > > > > I think this should be handled. It can be handled in either of
> > > > > > > the forms you show, or by saying:
> > > > > > > ```
> > > > > > > A Object1;
> > > > > > > A Object2;
> > > > > > > ```
> > > > > > > If all of these turn out to be a problem, we can still diagnose
> > > > > > > without providing a fixit.
> > > > > > >
> > > > > > > As for globals in general, they can be handled in a separate
> > > > > > > patch once we figure out the declaration grouping.
> > > > > > OK. I will try to split the object definition from the class
> > > > > > definition, as you have suggested. Thus, I can kick out the
> > > > > > tagDecl-matcher as well. If there is no easy way to do this, it
> > > > > > will be reported anyway but without a fixit.
> > > > > >
> > > > > > Note for me: Update documentation!
> > > > > What about
> > > > > ```
> > > > > struct S {
> > > > > } S1;
> > > > > ```
> > > > > I would like to report this too, since two names are being declared
> > > > > here. `S` and `S1`. What do you think?
> > > > ```
> > > > struct {
> > > > } nn1, nn2;
> > > > ```
> > > > Shall we ignore anonymous definitions?
> > > To be more precise: Warn and provide a fixit for `struct S {} S1`. Only
> > > warn for `struct {} nn1, nn2`.
> > > What about
> > >
> > > ```
> > > struct S {
> > > } S1;
> > > ```
> > > I would like to report this too, since two names are being declared here.
> > > S and S1. What do you think?
> >
> > I don't think that this should be diagnosed. For one, this is a relatively
> > common pattern (arguably more common than `struct S { } S1, S2;`), but
> > also, it names two very distinct entities (a type and a variable).
> >
> > > ```
> > > struct {
> > > } nn1, nn2;
> > >```
> > > Shall we ignore anonymous definitions?
> >
> > Yes, we basically have to.
> >
> Transforming
> ```
> typedef struct X { int t; } X, Y;
> ```
> to
> ```
> typedef struct X { int t; };
> typedef X X;
> typedef X Y;
> ```
> will be valid, but looks odd.
I am on the fence about this transformation -- it does not declare new objects,
but instead declares new types. A very common pattern in some libraries (such
as the Win32 SDK) is to declare:
```
typedef struct Blah {
} Blah, *PBlah, **PPBlah;
```
Because of that pattern, diagnosing the above typedef is likely to be very
chatty in some projects, and I don't know that the fixit provides much real
benefit for types.
At least for the CERT DCL04-C version of this rule, that code should not be
flagged. Do you think any of the variations of this check should flag it?
https://reviews.llvm.org/D27621
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28321: [CUDA] Add __declspec spellings for CUDA attributes.
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: > > > > For my own edification, do you have a link to some documentation of > > > > what CUDA attributes are spelled with `__declspec`? > > > I do not believe such documentation exists. It is an implementation > > > detail in the CUDA headers -- users never write `__attribute__((device))` > > > or `__declspec(__device__)`. Instead, they write `__device__`. > > Then why are we introducing `__declspec(__device__)` rather than a keyword > > attribute `__device__`? > > > > My biggest concern is: I would like to verify that these actually should be > > supported as a `__declspec` attribute. From my simple testing in MSVC, it > > does not appear to support `__declspec(__device__)`, but perhaps I am doing > > it wrong (I'm mostly unfamiliar with CUDA). If this isn't something MSVC > > supports, then it is the first attribute we're supporting with a __declspec > > spelling that is not actually a Microsoft attribute, which is something > > worth discussing. > > Then why are we introducing __declspec(__device__) rather than a keyword > > attribute __device__? > > `__device__` is a macro defined in the CUDA headers, which must include and > we are not able to modify. Okay, it makes sense as to why we can't have a keyword spelling, but it also doesn't answer why we need the `__declspec` spelling for it. Are you saying that there are CUDA headers out there where this attribute is spelled with `__attribute__` and others with `__declspec`? If so, then this change makes a bit more sense to me. Comment at: clang/include/clang/Basic/Attr.td:610 let LangOpts = [CUDA]; let Documentation = [Undocumented]; } jlebar wrote: > aaron.ballman wrote: > > jlebar wrote: > > > aaron.ballman wrote: > > > > Now would be a good time to add the documentation for these attributes, > > > > otherwise there's a lot less chance users will know what ways they can > > > > spell the attributes (or what the attribute do). > > > See above, these are an implementation detail. > > We can still document that we support the attributes under their macro > > names, or do users not typically think of these macros as being attributes? > > I am mostly concerned about discoverability of the attributes -- how is a > > user to know what Clang does or does not support? > > how is a user to know what Clang does or does not support? > > These macros are fundamental to CUDA support. The statement "you can compile > CUDA code with clang" will immediately imply to every CUDA developer in > existence the statement "you can use `__device__` in your code". Yet we add new CUDA attributes periodically, and CUDA comes out with new versions and new features. Looking at the CUDA docs, I see `__managed__`, but I don't see such an attribute in Clang. How is a user to know whether we do/don't support such a construct? https://reviews.llvm.org/D28321 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27621: [clang-tidy] check to find declarations declaring more than one name
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:
> aaron.ballman wrote:
> > firolino wrote:
> > > aaron.ballman wrote:
> > > > firolino wrote:
> > > > > firolino wrote:
> > > > > > firolino wrote:
> > > > > > > firolino wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > firolino wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > Why do we not want to match this?
> > > > > > > > > > If we decide, whether we transform
> > > > > > > > > > ```
> > > > > > > > > > class A {
> > > > > > > > > > } Object1, Object2;
> > > > > > > > > > ```
> > > > > > > > > > to
> > > > > > > > > > ```
> > > > > > > > > > class A {
> > > > > > > > > > } Object1,
> > > > > > > > > > Object2;
> > > > > > > > > > ```
> > > > > > > > > > or
> > > > > > > > > > ```
> > > > > > > > > > class A {
> > > > > > > > > > }
> > > > > > > > > > Object1,
> > > > > > > > > > Object2;
> > > > > > > > > > ```
> > > > > > > > > > I might consider adding support for it. Moreover, this kind
> > > > > > > > > > of definition is usually seen globally and I don't know how
> > > > > > > > > > to handle globals yet. See
> > > > > > > > > > http://lists.llvm.org/pipermail/cfe-dev/2015-November/046262.html
> > > > > > > > > I think this should be handled. It can be handled in either
> > > > > > > > > of the forms you show, or by saying:
> > > > > > > > > ```
> > > > > > > > > A Object1;
> > > > > > > > > A Object2;
> > > > > > > > > ```
> > > > > > > > > If all of these turn out to be a problem, we can still
> > > > > > > > > diagnose without providing a fixit.
> > > > > > > > >
> > > > > > > > > As for globals in general, they can be handled in a separate
> > > > > > > > > patch once we figure out the declaration grouping.
> > > > > > > > OK. I will try to split the object definition from the class
> > > > > > > > definition, as you have suggested. Thus, I can kick out the
> > > > > > > > tagDecl-matcher as well. If there is no easy way to do this, it
> > > > > > > > will be reported anyway but without a fixit.
> > > > > > > >
> > > > > > > > Note for me: Update documentation!
> > > > > > > What about
> > > > > > > ```
> > > > > > > struct S {
> > > > > > > } S1;
> > > > > > > ```
> > > > > > > I would like to report this too, since two names are being
> > > > > > > declared here. `S` and `S1`. What do you think?
> > > > > > ```
> > > > > > struct {
> > > > > > } nn1, nn2;
> > > > > > ```
> > > > > > Shall we ignore anonymous definitions?
> > > > > To be more precise: Warn and provide a fixit for `struct S {} S1`.
> > > > > Only warn for `struct {} nn1, nn2`.
> > > > > What about
> > > > >
> > > > > ```
> > > > > struct S {
> > > > > } S1;
> > > > > ```
> > > > > I would like to report this too, since two names are being declared
> > > > > here. S and S1. What do you think?
> > > >
> > > > I don't think that this should be diagnosed. For one, this is a
> > > > relatively common pattern (arguably more common than `struct S { } S1,
> > > > S2;`), but also, it names two very distinct entities (a type and a
> > > > variable).
> > > >
> > > > > ```
> > > > > struct {
> > > > > } nn1, nn2;
> > > > >```
> > > > > Shall we ignore anonymous definitions?
> > > >
> > > > Yes, we basically have to.
> > > >
> > > Transforming
> > > ```
> > > typedef struct X { int t; } X, Y;
> > > ```
> > > to
> > > ```
> > > typedef struct X { int t; };
> > > typedef X X;
> > > typedef X Y;
> > > ```
> > > will be valid, but looks odd.
> > I am on the fence about this transformation -- it does not declare new
> > objects, but instead declares new types. A very common pattern in some
> > libraries (such as the Win32 SDK) is to declare:
> > ```
> > typedef struct Blah {
> >
> > } Blah, *PBlah, **PPBlah;
> > ```
> > Because of that pattern, diagnosing the above typedef is likely to be very
> > chatty in some projects, and I don't know that the fixit provides much real
> > benefit for types.
> >
> > At least for the CERT DCL04-C version of this rule, that code should not be
> > flagged. Do you think any of the variations of this check should flag it?
> Maybe we should reconsider where to split and where not. The original idea
> was to minimize confusion on code like
> ```
> int* a,b;
> ```
> whereas
> ```
> struct S {} *s1, s2;
> ```
> seems not so confusing as above. However,
> ```
> struct S {}* s1, s2;
> ```
> looks dangerous again.
>
> Even following code looks unfamiliar to me.
> ```
> typedef struct Blah {} Blah, *PBlah;
> ```
>
> It really depends on the beholder. So, how about letting this check split
> **everything** and provide options for maximum flexibility? So specific
> rulesets or the user may configure it as they wish. We could add for example
>
[PATCH] D28287: [clang-tidy] Ignore default arguments in modernize-default-member-init
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: clang-tidy/modernize/UseDefaultMemberInitCheck.cpp:161
cxxBoolLiteral(), cxxNullPtrLiteralExpr(), implicitValueInitExpr(),
-declRefExpr());
+declRefExpr(unless(to(varDecl();
Any `varDecl`? Or do we want to limit it to only `parmVarDecl` instead?
```
int i = 12;
struct S {
int j;
S() : j(i) {}
};
```
It seems reasonable to suggest the member initialization be "fixed" to: `int j
= i;`
https://reviews.llvm.org/D28287
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28287: [clang-tidy] Ignore default arguments in modernize-default-member-init
aaron.ballman added inline comments.
Comment at: clang-tidy/modernize/UseDefaultMemberInitCheck.cpp:161
cxxBoolLiteral(), cxxNullPtrLiteralExpr(), implicitValueInitExpr(),
-declRefExpr());
+declRefExpr(unless(to(varDecl();
malcolm.parsons wrote:
> aaron.ballman wrote:
> > Any `varDecl`? Or do we want to limit it to only `parmVarDecl` instead?
> > ```
> > int i = 12;
> > struct S {
> > int j;
> > S() : j(i) {}
> > };
> > ```
> > It seems reasonable to suggest the member initialization be "fixed" to:
> > `int j = i;`
> I would find a default member initializer that is not a constant to be
> surprising.
> Maybe I want to limit it to `enumConstantDecl`.
I wouldn't; consider a global synchronization object, like a mutex (this is a
relatively common thing).
https://reviews.llvm.org/D28287
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D28166: Properly merge K functions that have attributes
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
include/clang/AST/TypeLoc.h
lib/Sema/SemaDecl.cpp
test/Sema/knr-def-call.c
test/Sema/warn-strict-prototypes.c
Index: test/Sema/warn-strict-prototypes.c
===
--- test/Sema/warn-strict-prototypes.c
+++ test/Sema/warn-strict-prototypes.c
@@ -60,3 +60,8 @@
// K function definition with previous prototype declared is not diagnosed.
void foo11(int p, int p2);
void foo11(p, p2) int p; int p2; {}
+
+// PR31020
+void __attribute__((cdecl)) foo12(d) // expected-warning {{this old-style function definition is not preceded by a prototype}}
+ short d;
+{}
Index: test/Sema/knr-def-call.c
===
--- test/Sema/knr-def-call.c
+++ test/Sema/knr-def-call.c
@@ -39,3 +39,9 @@
proto(42.1); // expected-warning{{implicit conversion from 'double' to 'int' changes value from 42.1 to 42}}
()(42.1); // expected-warning{{implicit conversion from 'double' to 'int' changes value from 42.1 to 42}}
}
+
+// PR31020
+void func(short d) __attribute__((cdecl)); // expected-note{{previous declaration is here}}
+void __attribute__((cdecl)) func(d)
+ short d; // expected-warning{{promoted type 'int' of K function parameter is not compatible with the parameter type 'short' declared in a previous prototype}}
+{}
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -7458,11 +7458,12 @@
// Determine whether the function was written with a
// prototype. This true when:
// - there is a prototype in the declarator, or
-// - the type R of the function is some kind of typedef or other reference
-// to a type name (which eventually refers to a function type).
+// - the type R of the function is some kind of typedef or other non-
+// attributed reference to a type name (which eventually refers to a
+// function type).
bool HasPrototype =
(D.isFunctionDeclarator() && D.getFunctionTypeInfo().hasPrototype) ||
- (!isa(R.getTypePtr()) && R->isFunctionProtoType());
+ (!R->getAsAdjusted() && R->isFunctionProtoType());
NewFD = FunctionDecl::Create(SemaRef.Context, DC,
D.getLocStart(), NameInfo, R,
@@ -11950,7 +11951,7 @@
!LangOpts.CPlusPlus) {
TypeSourceInfo *TI = FD->getTypeSourceInfo();
TypeLoc TL = TI->getTypeLoc();
-FunctionTypeLoc FTL = TL.castAs();
+FunctionTypeLoc FTL = TL.getAsAdjusted();
Diag(FTL.getLParenLoc(), diag::warn_strict_prototypes) << 1;
}
}
Index: include/clang/AST/TypeLoc.h
===
--- include/clang/AST/TypeLoc.h
+++ include/clang/AST/TypeLoc.h
@@ -70,6 +70,26 @@
return t;
}
+ /// \brief Convert to the specified TypeLoc type, returning a null TypeLoc if
+ /// this TypeLock is not of the desired type. It will consider type
+ /// adjustments from a type that wad written as a T to another type that is
+ /// still canonically a T (ignores parens, attributes, elaborated types, etc).
+ template
+ T getAsAdjusted() const {
+TypeLoc Cur = *this;
+while (!T::isKind(Cur)) {
+ if (auto PTL = Cur.getAs())
+Cur = PTL.getInnerLoc();
+ else if (auto ATL = Cur.getAs())
+Cur = ATL.getModifiedLoc();
+ else if (auto ETL = Cur.getAs())
+Cur = ETL.getNamedTypeLoc();
+ else
+break;
+}
+return Cur.getAs();
+ }
+
/// The kinds of TypeLocs. Equivalent to the Type::TypeClass enum,
/// except it also defines a Qualified enum that corresponds to the
/// QualifiedLoc class.
Index: include/clang/AST/Type.h
===
--- include/clang/AST/Type.h
+++ include/clang/AST/Type.h
@@ -1875,6 +1875,13 @@
/// immediately following this class.
template const T *getAs() const;
+ /// Member-template getAsAdjusted. Look through specific kinds
+ /// of sugar (parens, attributes, etc) for an instance of \.
+ /// This is used when you need to walk over sugar nodes that represent some
+ /// kind of type adjustment from a type that was written as a \
+ /// to another type that is still canonically a \.
+ template const T *getAsAdjusted() const;
+
/// A variant of getAs<> for array types which silently discards
/// qualifiers from the outermost type.
const ArrayType *getAsArrayTypeUnsafe() const;
@@ -5932,6 +5939,36 @@
return cast(getUnqualifiedDesugaredType());
}
+template const T *Type::getAsAdjusted() const {
+ static_assert(!TypeIsArrayType::value, "ArrayType
[PATCH] D28321: [CUDA] Add __declspec spellings for CUDA attributes.
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 means required for this patch to go in. https://reviews.llvm.org/D28321 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D27424: Add the diagnose_if attribute to clang.
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 cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D20693: [clang-tidy] New checker to replace dynamic exception specifications
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/
> s/throw(something)//
>
>
> Aaron, does this work for you?
I think it makes a reasonable option, but I think it should be off by default.
Again, I think that's hostile towards users to remove an explicit exception
specification entirely when there was an explicit dynamic exception
specification written on the function signature. `noexcept(false)` is a
stronger signal to anyone reading the signature than no exception specification
whatsoever.
Be careful, though, not to break code by removing the exception specification.
These two are *not* equivalent, for instance:
struct S {
~S() noexcept(false);
void operator delete(void *ptr) noexcept(false);
};
struct T {
~T(); // Is actually noexcept(true) by default!
void operator delete(void *ptr); // Is actually noexcept(true) by default!
};
https://reviews.llvm.org/D20693
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D21298: [Clang-tidy] delete null check
aaron.ballman added a comment. In https://reviews.llvm.org/D21298#632154, @xazax.hun wrote: > Small ping, is this ready to be committed? If @alexfh doesn't sign off by tomorrow, I think it's fine to commit. We can deal with any last minute comments post-commit. https://reviews.llvm.org/D21298 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
