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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/AttrDocs.td:1007
+
+defined_in=\ *string-literal*
+  The name of the source container in which the declaration was defined. The
----------------
arphaman wrote:
> aaron.ballman wrote:
> > Would this hold 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