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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
