[PATCH] D107696: [CodeComplete] Basic code completion for attribute names.

[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.

Btw, I'm not certain why patch application is failing for you in the precommit CI: https://buildkite.com/llvm-project/diff-checks/builds/58571#c186a7d3-f5c9-4ad2-ae27-07408b1c5dad It doesn't seem like your patch is the cause of it, but this review is the only one where I've noticed the issue. Perhaps if you upload a new patch CI will be happier? Otherwise, beware of bot stampedes when you land. :-)

This LGTM (if you want to, feel free to switch to `Twine` in the one place I called out, but we don't need another round of review for that), thank you for the patch!



================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4390
+  auto AddCompletions = [&](const ParsedAttrInfo &A) {
+    if (A.IsTargetSpecific && !A.existsInTarget(Context.getTargetInfo()))
+      return;
----------------
sammccall wrote:
> aaron.ballman wrote:
> > Should we also early return if the attribute is ignored? (See `IgnoredAttr` in `Attr.td`) I'm assuming that attributes which do nothing are unlikely to be high-value attributes to autocomplete (so maybe they'd go at the end of the list if we wanted to keep them).
> Hmm, I'm not sure about that.
> They do nothing *in clang*, but using clang-based code completion doesn't particularly mean we'll use clang to build the code.
> Something must care about the attribute, even if it's just a doc generator or something.
> 
> In practice, there are 6 attrs with Ignored = 1:
>  - `bounded` is an openbsd-gcc extension
>  - 3 are cuda-specific and probably only used by cuda stdlib headers
>  - `__w64` is a keyword so not relevant here
>  - `property` is actually supported by clang, it seems `Ignored` is lying!
> 
> So none of these are *terribly* important, but they also don't seem so worthless that it's important to exclude them. (There are other attributes that aren't important too!)
> Hmm, I'm not sure about that.

I'm convinced by your reasoning, thank you!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107696