[PATCH] D141324: [clang] extend external_source_symbol attribute with the USR clause

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I'm disturbed that the string-literal diagnostic you changed never shows up in the tests.  I suspect this attribute needs significantly better test coverage.



================
Comment at: clang/docs/LanguageExtensions.rst:4815
+
+An optional USR string literal can be added to the ``external_source_symbol``
+attribute. For example:
----------------
Can you explain in here what the "USR" is here/means here?  Or what said string is supposed to be?


================
Comment at: clang/docs/LanguageExtensions.rst:4823
+Query for this feature with
+``__has_feature(attribute_external_source_symbol_with_usr)``.
----------------
`__has_feature` isn't really the right solution here.  Though, we don't typically have a way to test for individual features of an attribute other than the value that `__has_attribute` returns.  In standard attributes, it returns a larger number when new features are added (see SD-10's Feature Test Macros).

I don't think I want us using `__has_feature` here though, either a different value for `__has_attribute` might be OK, or alternatively, perhaps just a simple feature-test-macro.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:1754
+USR=\ *string-literal*
+  The exact USR of the foreign symbol. When not specified, Clang's
+  indexer will use the Clang USR for this symbol.
----------------
This DEFINITELY needs to better explain what a USR is, and why one would use it.  Many times this document is used by folks to discover attributes they didn't know existed, and this needs to be usable for someone who doesn't really get the context until reading this document.


================
Comment at: clang/include/clang/Basic/DiagnosticCommonKinds.td:58
           "for optional message in 'availability' attribute|"
-          "for %select{language|source container}1 name in "
+          "for %select{language name|source container name|USR}1 in "
           "'external_source_symbol' attribute}0">;
----------------
I don't think adding 'name' to these makes them particularly more readable?  At least from what I can see reading it here.  


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141324/new/

https://reviews.llvm.org/D141324