[PATCH] D47223: [clangd] Handle enumerators in named, unscoped enums similarly to scoped enums

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D47223#1110571, @malaperle wrote:

> In https://reviews.llvm.org/D47223#1109247, @ilya-biryukov wrote:
>
> > I'm not sure if we have tests for that, but I remember that we kept the enumerators in the outer scope so that completion could find them..
> >  Am I right that this patch will change the behavior and we won't get enumerators in the following example:
> >
> >   /// foo.h
> >   enum Foo {
> >     A, B, C
> >   };
> >  
> >   /// foo.cpp
> >   #include "foo.h"
> >  
> >   int a = ^ // <-- A, B, C should be in completion list here.
> >
>
>
> Not quite but still potentially problematic. With the patch, A, B, C would be found but not ::A, ::B, ::C.
>
> > It's one of those cases where code completion and workspace symbol search seem to want different results :-(
> >  I suggest to add an extra string field for containing unscoped enum name, maybe into symbol details? And add a parameter to `Index::fuzzyFind` on whether we need to match enum scopes or not.
> >  + at ioeric, + at sammccall,  WDYT?
>
> I'll wait to see what others think before changing it. But I feel it's a bit odd that completion and workspace symbols would be inconsistent. I'd rather have it that A, ::A, and Foo::A work for both completion and workspace. Maybe it would complicate things too much...


Having "wrong" names in workspaceSymbol (e.g. ::A instead of ::Foo::A) and missing results in code completions (e.g. ::A missing in ::^) seem equally bad.

I think the fundamental problem here is that C++ symbols (esp. enum constants) can have different names (e.g. ns::Foo::A and ns::A). We want `ns::Foo::A` for workspaceSymbols, but we also want `ns::A` for index-based code completions (sema completion would give us `ns::Foo::A` right? ). One possible solution would be adding a short name in `Symbol` (e.g. `ns::A` is a short name for `ns::Foo::A`), and index/fuzzyFind can match on both names.



================
Comment at: clangd/index/SymbolCollector.cpp:365
+
+  using namespace clang::ast_matchers;
+  // For enumerators in unscoped enums that have names, even if they are not
----------------
drive-by nit: please pull this to a helper function.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D47223