aaron.ballman added inline comments.
================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4390
+ auto AddCompletions = [&](const ParsedAttrInfo &A) {
+ if (A.IsTargetSpecific && !A.existsInTarget(Context.getTargetInfo()))
+ return;
----------------
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).
================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4436-4446
+ llvm::SmallString<32> Guarded;
+ if (!Scope.empty()) {
+ const char *GuardedScope = underscoreAttrScope(Scope);
+ if (!GuardedScope)
+ continue;
+ Guarded.append(GuardedScope);
+ Guarded.append("::");
----------------
This could probably use a `Twine` and save some typing, but I don't insist.
================
Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4357
+ continue;
+ llvm::StringRef Name = S.NormalizedFullName;
+ if (Completion == AttributeCompletion::Scope) {
----------------
sammccall wrote:
> aaron.ballman wrote:
> > sammccall wrote:
> > > aaron.ballman wrote:
> > > > Should we also add some special handling for attributes optionally
> > > > starting with double underscores? e.g., `__attribute__((const))` and
> > > > `__attribute__((__const__))`` are both equally useful to complete.
> > > >
> > > > Also, we should add some explicit testing for `omp::sequence` and
> > > > `omp::directive` as those are handled very specially and won't appear
> > > > in the parsed attribute map. I think the OpenMP code completion would
> > > > be super useful, but can be handled in a follow-up if you want.
> > > > Should we also add some special handling for attributes optionally
> > > > starting with double underscores?
> > >
> > > I think so. Two questions:
> > > - Do I understand right that this is "just" a matter of adding
> > > leading/trailing `__` as a second option, for AS_GNU?
> > > - are there similar rules for other syntaxes I've missed?
> > >
> > > Offering both seems potentially confusing for users who don't care
> > > (especially in the case of const!). But I guess enough users will care
> > > about macros. At least in clangd the underscore versions will get ranked
> > > lower for having "internal" names though.
> > >
> > > FWIW The no-underscores version appears to be ~3x more common (87k vs 27k
> > > hits in third-party code in google's repo). Among headers, no-underscores
> > > is "only" 2x more common (40k vs 21k).
> > >
> > > ---
> > >
> > > > Also, we should add some explicit testing for omp::sequence and
> > > > omp::directive as those are handled very specially and won't appear in
> > > > the parsed attribute map.
> > >
> > > Yeah, I punted on these because it seems they will need special case
> > > logic, I'll add some tests that they don't do anything.
> > > Do I understand right that this is "just" a matter of adding
> > > leading/trailing __ as a second option, for AS_GNU?
> > > are there similar rules for other syntaxes I've missed?
> >
> > Clang supports GNU attributes in either `__attribute__((foo))` or
> > `__attribute__((__foo__))` forms. So I'd say that autocompleting after the
> > second `(` should either suggest attributes (preferred) or `__` (for the
> > poor folks writing libraries). If the user wants to autocomplete after
> > `__attribute__((__`, I think it should suggest `foo__` as the rest of the
> > attribute name. (Basically, if the user looks like they want underscores,
> > give them all the underscores.)
> >
> > Clang also supports `[[]]` attributes but with somewhat different rules. We
> > support `[[gnu::attr]]`, `[[__gnu__::attr]]`, `[[gnu::__attr__]]`, and
> > `[[__gnu__::__attr__]]` for GCC attributes. We support `[[clang::attr]]`,
> > `[[_Clang::attr]]`, `[[clang::__attr__]]`, and `[[_Clang::__attr__]]` for
> > Clang attributes. For vendors other than Clang and GCC, we don't support
> > any additional underscores for either the scope or the attribute name. I
> > would say that if the user asked for underscores for the vendor scope, they
> > likely want the underscores for the attribute as well.
> >
> > I suppose there's a third case. That horrible `using` syntax that I've
> > never really seen used in the wild. e.g., ``[[using clang: attr]``. We do
> > support the underscore behavior there as well.
> >
> > > Offering both seems potentially confusing for users who don't care
> > > (especially in the case of const!). But I guess enough users will care
> > > about macros.
> >
> > Yeah, users who are writing portable libraries are far more likely to care
> > than users writing typical application code.
> > So I'd say that autocompleting after the second ( should either suggest
> > attributes (preferred) or __ (for the poor folks writing libraries).
>
> This is clever, unfortunately it doesn't really work for other reasons:
> - LSP allows clients to "narrow down" results as an identifier is typed with
> client-side filtering, and `_` is treated as an identifier character.
> - clang's code completion similarly returns all matches and leaves
> partial-identifier-filtering them to the CodeCompleteConsumer
>
> I can't see a better plan than including both the underscore and
> non-underscore versions, with the latter downranked. Doesn't seem *too* bad.
>
> > Clang also supports [[]] attributes...
>
> Yeah, I think what we should do here is pretend the scoped namespace contains
> only the scoped attribute and vice versa:
> - When we want qualified attributes, emit `gnu::attr` and
> `__gnu__::__attr__` but no mixing
> - When we want scopes (after `using`), emit `gnu` and `__gnu__`
> - When we want unqualified attributes (after `ns::` or `using ns:`), emit
> either `attr` or `__attr__` depending on the scope
>
> > That horrible using syntax
>
> I really don't understand why this is worthwhile. Maybe feature parity with
> XMLNS? :-)
> In any case, it happens to be easy to handle.
> I can't see a better plan than including both the underscore and
> non-underscore versions, with the latter downranked. Doesn't seem *too* bad.
Seems reasonable enough to me!
> Yeah, I think what we should do here is pretend the scoped namespace contains
> only the scoped attribute and vice versa:
+1, that seems good to me.
> I really don't understand why this is worthwhile.
IMHO, it absolutely wasn't worthwhile. :-D
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
