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

Reply via email to