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<IgnoredAttributes>;
----------------
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 &Attr) {
+ 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 some of them.
The test is still missing the weird parsing situations (like failing to use an
`=` for arguments, etc). Basically, you should look at your custom parsing code
and make sure each case you want to diagnose is actually being diagnosed (so we
don't accidentally regress if we ever refactor the parsing code).
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
