[PATCH] D67826: [clangd] A helper to find explicit references and their names

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

I believe all comments were addressed. PTAL and let me know if I missed something.



================
Comment at: clang-tools-extra/clangd/FindTarget.h:93
+  /// in 'obj.foo'.
+  const Expr* MemberExprBase = nullptr;
+};
----------------
kadircet wrote:
> do we have any real use-case for this field? apart from "skipping instance members in define inline"? because that one can also be achieved easily through decl (there is a `isCXXInstanceMember`).
Thanks, this is definitely a valid concern. I was overthinking the problem, removed it.


================
Comment at: clang-tools-extra/clangd/FindTarget.h:115
+/// FIXME: extend to report location information about declaration names too.
+void findExplicitReferences(Stmt *S,
+                            llvm::function_ref<void(ReferenceLoc)> Out);
----------------
kadircet wrote:
> It might be better to accept a `DynTypedNode` in here as well, so that this can be used with other node types as well, or provide overrides for them(for example `Decl`).
I would avoid doing `DynTypedNode` - many of the cases don't quite make sense here, e.g. `TypeLoc` is arguably ok as it has location information, but `Type*` or `QualType` are not ok as they lack location information.

`Decl` definitely sounds useful, though. Would allow to visit all top-level decls of a file, which is another use-case we probably want. Added that.


================
Comment at: clang-tools-extra/clangd/FindTarget.h:96
+};
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, ReferenceLoc R);
+
----------------
kadircet wrote:
> ilya-biryukov wrote:
> > kadircet wrote:
> > > Is this for testing purposes? maybe move it into `findtargettests.cpp` or make it a member helper like `Print(SourceManager&)` so that it can also print locations etc.?
> > Yes, this is for output in the test. Moving to `findTargetTests.cpp`, we risk ODR violations in the future.
> > In any case, it's useful to have debug output and `operator<<` is common for that purpose: it enables `llvm::toString` and, consecutively, `llvm::formatv("{0}", R)`.
> > 
> > What are the downsides of having it?
> > 
> I wasn't happy about it because it actually needs a `SourceManager` to print completely, and I believe that's also necessary for debugging.
> 
> But feel free to ignore if you're OK with that.
Having some `llvm::toString` is great, because it's nicely integrated to all of the infrastructure and very easy to use.
I'll stick with it for now, despite the limitations arising from the absence of source manager.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67826