[PATCH] D136925: [clangd] Index scoped enums for code completion

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.


================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:2133
   if (const auto *EnumDecl = dyn_cast<clang::EnumDecl>(ND.getDeclContext()))
-    return InTopLevelScope(*EnumDecl) && !EnumDecl->isScoped();
 
----------------
tom-anders wrote:
> nridge wrote:
> > Just to make sure I understand:
> > 
> > By also removing the `!isScoped()` condition, in addition to changing the behaviour for the scenario described in https://github.com/clangd/clangd/issues/1082 (enum declared at class scope), you are also changing the behaviour for scenarios like this:
> > 
> > ```
> > enum class Foo { Bar };  // at any scope, including global
> > ```
> > 
> > Completing `Bar` will now offer `Foo::Bar` when previously it didn't.
> > 
> > Is this your intention?
> Ah sorry, that is indeed not what I described in the issue - but yeah this change is intended here,  IMO it's more consistent from a user perspective: Why should **all-scopes**-completion ignore **scoped** enums?
I have no objection to this additional change personally, but to play devil's advocate a bit: one could argue that for scoped enums, because the enumeration name needs to be mentioned at every use, the enumerator names are more likely to be shorter/common words, such that they may appear in multiple enumerations across a project, making the completion results more noisy.

Perhaps, it would make sense to split this out into a separate patch, so that in case it turns out to be controversial, it can be reverted without affecting the other part of the fix?


================
Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:1334
+                  AllOf(qName("Color3"), forCodeCompletion(true)),
+                  AllOf(qName("Color3::Blue"), forCodeCompletion(true)),
                   AllOf(qName("ns"), forCodeCompletion(true)),
----------------
I think it would be nice to add a test case to `CodeCompleteTests.cpp` as well.

(For example, it was not obvious to me that in the scoped enum case, completing the enumerator name would correctly insert the enumeration name as part of the scope, until I actually tried it in the editor.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136925