hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.
================
Comment at: clangd/XRefs.cpp:38
+ // Only a single declaration is allowed.
+ if (isa<ValueDecl>(D)) // except cases above
+ return D;
----------------
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?
================
Comment at: unittests/clangd/XRefsTests.cpp:339
+ if (!T.ranges("def").empty())
+ WantDecl = T.range("def");
+
----------------
hokein wrote:
> I think this should be `WantDef`?
what about this comment?
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57388/new/
https://reviews.llvm.org/D57388
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
