sammccall marked 2 inline comments as done.
sammccall added inline comments.
================
Comment at: clangd/XRefs.cpp:38
+ // Only a single declaration is allowed.
+ if (isa<ValueDecl>(D)) // except cases above
+ return D;
----------------
hokein wrote:
> sammccall wrote:
> > hokein wrote:
> > > is this a case that we were missing before? can we add a unittest for it
> > > (I didn't find a related change in the unittest).
> > Previously, this function only had to be correct for things that can be
> > declared and defined separately.
> > For some decls that are always definitions (e.g. member variables) we would
> > return nullptr here, and treat them as decl-only ... but that was OK,
> > because the return value was just "a list of locations" and it didn't
> > matter whether we treated them as decls or defs when there's just one.
> >
> > However now the return type is "here's the decl, and here's the def".
> > Without this change, we have Definition == llvm::None for e.g. member
> > variables. While the LSP method falls back from def->decl so you probably
> > can't observe this problem, API users can.
> >
> > This is in fact covered by the tests, though it's kind of indirect (this is
> > a private helper function, after all).
> >
> Thanks for the explanation. Maybe add more in the comment?
Added a little bit - there's not really so much to say, because the new
behavior is kind of the obvious one.
The old behavior ("just return nullptr if it doesn't matter") maybe deserved a
longer comment, but we didn't notice.
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57388/new/
https://reviews.llvm.org/D57388
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
