[PATCH] D128826: Go-to-type on smart_ptr<Foo> now also shows Foo

[Tom Praschan via Phabricator via cfe-commits]

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