tom-anders added inline comments.
================
Comment at: clang-tools-extra/clangd/HeuristicResolver.h:75
+ // could look up the name appearing on the RHS.
+ const Type *getPointeeType(const Type *T) const;
+
----------------
sammccall wrote:
> tom-anders wrote:
> > Not sure if it's the right call to make this public? The documentation
> > needs to be adapted at least I think
> It's a bit unfortunate that it doesn't fit the usual pattern of "resolve the
> target of this AST node", but it's fairly closely related, it needs to use
> the Ctx in the same way, and it's already implemented here... I think it's OK
> (at least I can't see a better alternative).
>
> We should probably have a unit test for it though.
Not really sure where to put the unit test, probably FindTargetTests.cpp? It
looks like it tests `HeuristicResolver` implicitly. Couldn't find anything that
tests `HeuristicResolver` directly.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1913
// to target.
-static QualType unwrapFindType(QualType T) {
+static llvm::SmallVector<QualType> unwrapFindType(
+ QualType T, const HeuristicResolver* H) {
----------------
sammccall wrote:
> Ergonomically I might prefer `void unwrapFindType(QualType, ...,
> vector<QualType>& Out)` or so.
> This tends to compose a bit better IMO by making all the cases look similar
> whether you're adding one type or several or combining lists.
>
> You can still `return Out.push_back(T)` etc on a single line.
Ah yes that makes sense. Didn't think about the `return Out.push_back(T)` trick
to keep using early returns, thought I'd have to add a ton of `else if`s
instead.
================
Comment at: clang-tools-extra/clangd/XRefs.cpp:1966
+ if (!Type.isNull()) {
+ llvm::copy(locateSymbolForType(AST, Type),
std::back_inserter(LocatedSymbols));
+ }
----------------
sammccall wrote:
> we now have the potential for duplicates (already in this patch in fact:
> unique_ptr<unique_ptr<int>>.
>
> Deduplicating types isn't that useful (unique_ptr<unique_ptr<int>> !=
> unique_ptr<int>), I guess it's either deduplicate locations or allow the
> duplicates.
> If you choose to allow them, probably worth a comment why.
Not sure about this either. I'll go with not deduplicating and adding a comment
for now.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D128826/new/
https://reviews.llvm.org/D128826
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
