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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

This generally LGTM but the only other thing I'm wondering is whether there should be any clang-specific testing for this functionality. Right now, it relies on running the clangd unit tests to notice failures and that's not ideal because not everyone builds clang-tools-extra as part of their usual Clang development. However, given that this functionality isn't really *used* by Clang, I'm not certain there's much to test and there's not a whole lot of failure mode beyond "the file didn't get generated".

So giving this my LGTM, but if you think of a way to introduce clang-specific tests for this, I'm happy to do further review.



================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:4220
+  )cpp";
+  std::vector<Record *> Attrs = Records.getAllDerivedDefinitions("Attr");
+  for (const auto *A : Attrs) {
----------------
sammccall wrote:
> aaron.ballman wrote:
> > FWIW, this will miss `omp::sequence` and `omp::directive`, but that's not the end of the world. May be worth a fixme comment in case we want to solve it someday.
> Well, those
>  - don't themselves have Attr nodes or AttrKinds, so the API couldn't query for them
>  - don't actually have any documentation as far as I can tell
> So I'm not sure there's much to fix.
> 
> ---
> 
> The ParsedAttr-kind vs Attr-kind distinction, and the fact that many attributes don't leave any AST nodes at all, mean this method doesn't seem "universal". But the goal is to show docs in clangd when hovering on an attribute (which we only support for those with AST nodes).
> 
> If we want to do something different (like show documenation while code-completing) we'll probably have to add a second ParsedAttrKind-based API.
> So I'm not sure there's much to fix.

The big reason they're not documented is because I've not thought of a good way to expose the documentation via AttrDocs.td. Currently, that documentation needs the attributes to live in Attr.td, and we don't expose the attributes there because we have no way to express their arguments.

At some point in the future, I expect we'll make these two attributes "more normal" by getting them into Attr.td and AttrDocs.td, and at that point this probably fixes itself? So I think you're right, maybe not much to fix *here*.




================
Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:4242
+  llvm::StringRef clang::Attr::getDocumentation(clang::attr::Kind K) {
+    assert(K < llvm::array_lengthof(AttrDoc));
+    return AttrDoc[K];
----------------
sammccall wrote:
> aaron.ballman wrote:
> > Hmmmm, I am not 100% certain this assertion is correct -- the user may have plugin attributes, which I believe are given a "kind" that's larger than the last-known builtin attribute kind.
> Thanks, changed it to an if() instead.
> (I don't think it's particularly pressing to allow them to be documented)
Agreed that we can figure out how to let plugin attributes add documentation at a later date.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107703