[PATCH] D153248: [clangd] Use resolveTypeToRecordDecl() to resolve the type of a base specifier during heuristic resolution

Mon Jun 19 03:00:41 PDT 2023

hokein added a comment.

In D153248#4431780 <https://reviews.llvm.org/D153248#4431780>, @nridge wrote:

> As this patch involves copying a bit of code from CXXRecordDecl::lookupDepenentName() to HeuristicResolver, I wanted to mention a couple of alternatives I considered:
>
> 1. Copy things in the other direction, i.e. implement some of the necessary parts of resolveTypeToRecordDecl() in the helpers of lookupDependentName() instead. I decided against this because HeuristicResolver is an area of the code that's actively being improved; instead of having to make multiple updates to lookupDependentName() over time, it seems cleaner to just break HeuristicResolver's dependency on that function.
> 2. Wait to make this change until after HeuristicResolver has been upstreamed <https://github.com/clangd/clangd/discussions/1662> and unified with lookupDependentName(). I do plan to work on this upstreaming, but as the process will involve several steps (e.g. writing new tests, getting approval from relevant upstream maintainers), I would prefer not to block this change on the resolution of that process.

Thanks for all thoughtful considerations.
It is a bit sad that we duplicate the implementation in the `HeuristicResolver`, but given that

- this part of code is not large
- we need it in the Resolver anyway when upstreaming to clang

I think it is probably OK to do that.

The other question might worth thinking, are these cases critical to fix now? I'm not sure `this->find()` is a common case (`find();` already works today).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153248