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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
