[PATCH] D88472: [WIP][clangd] Support non-renaming alias underlying decls in TargetDecl.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

We manage to get rid of a little code here, but we add complexity to an important API, and make... one test better and a few tests worse.
I'd like to get rid of the wart in the code, but the tradeoff doesn't seem completely compelling... going to think about the API here a bit.



================
Comment at: clang-tools-extra/clangd/FindTarget.h:112
+  /// Underlying declarations for renaming alias (typedef decl, type alias decl)
+  AliasUnderlying,
+  /// Underlying declarations for non-renaming alias, decltype, etc.
----------------
hokein wrote:
> The previous `Alias` vs `Underlying` were pretty nice, but they were not enough to support our non-renaming-alias-underlying case. Looking for the feedback on the API change here.  
Inevitably this adds some noise, and the names are less clear (we certainly need to avoid using "alias" to mean two different things, and we should try to avoid negatives that confuse the boolean logic).
We could simplify this at least at query locations by defining Underlying = NonAliasUnderlying | AliasUnderlying, but unfortunately we can't put it in DeclRelation.

The other thing is this isn't complete either, because it doesn't distinguish between renaming and non-renaming `Alias`es. This is why we have the regression in the testcases.

That this is so verbose is a property of the original design: the idea was "mark this edge as alias->underlying, and apply a bit when that edge is traversed" but of course we decided to signal "the edge is not traversed" too. So if we introduce a new type of edge, we need *two* new bits - one at each end.

I'm trying to think of ideas for improvements without totally ditching the API...


================
Comment at: clang-tools-extra/clangd/unittests/XRefsTests.cpp:1120
       namespace ns { class [[Foo]] {}; }
-      using ns::F^oo;
+      using ns::[[F^oo]];
     )cpp",
----------------
hokein wrote:
> this seems a small regression, I think it is fine.
I can't follow why this is different from the second case of

TEST_F(TargetDeclTest, UsingDecl), where the usingdecl is not reported at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88472