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