[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 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 &Attr) {
+  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<NamedDecl>(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