aaron.ballman added inline comments.
================ Comment at: clang/lib/Sema/SemaCodeComplete.cpp:4357 + continue; + llvm::StringRef Name = S.NormalizedFullName; + if (Completion == AttributeCompletion::Scope) { ---------------- 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. 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