[PATCH] D107703: [AST][clangd] Expose documentation of Attrs on hover.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 9 inline comments as done.
sammccall added inline comments.


================
Comment at: clang/unittests/AST/AttrTest.cpp:19
+TEST(Attr, Doc) {
+  EXPECT_THAT(Attr::getDocumentation(attr::Used),
+              testing::HasSubstr("The compiler must emit the definition even "
----------------
aaron.ballman wrote:
> It seems this is failing the premerge CI -- I think you need a call to `.str()` in here to convert the `StringRef` to a `std::string`?
Thanks - no idea why this only breaks MSVC, but gmock is a subtle beast...


================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:4237
+  static const llvm::StringRef AttrDoc[] = {
+  #define ATTR(NAME) AttrDoc_##NAME,
+  #include "clang/Basic/AttrList.inc"
----------------
aaron.ballman wrote:
> kadircet wrote:
> > i am not well-versed in tablegen, so sorry if i am being dense here, butt what happens to the attributes we skipped above (e.g. the ones without a `ASTNode` flag)?
> > 
> > Maybe we should be emitting a `... AttrDoc_X[] = "";` for all attributes, no matter what?
> The ones without an AST node don't get an `attr::Kind` enumeration generated for them, so I believe this interface is safe.
Right, all those extra strings would just tickle -Wunused :-(

I'd love to have an API that can also be used for attrs with ASTNode=0 (many are documented!). However:
 - for the motivating use case (hover), we can only trigger when we find an AST node anyway (sadly)
 - the obvious alternative is AttributeCommonInfo::Kind, (i.e. ParsedAttr kind), which isn't actually better, just differently bad. e.g. there's one ParsedAttr kind for `interrupt`, but three target-specific `Attr` kinds, each of which is documented separately. For hover you want to get the right one, and ParsedAttr kind can't choose it.


================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:4231
+      // Only look at the first documentation if there are several.
+      // (As of now, only one attribute has multiple documentation entries).
+      break;
----------------
kadircet wrote:
> not sure if this comment will stay useful.
I want a comment to avoid a chesterton's fence:
 - the motivation for doing something lazy is that this is really rare
 - it's sensible to revisit this if it stops being rare

Reworded it to make this more explicit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107703