This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Be more explicit on testing the optional DefLoc in LocatedSymbol.
ClosedPublic

Authored by hokein on Jul 30 2020, 1:47 AM.

Details

Summary

And also fix a bug where we may return a meaningless location.

Diff Detail

Event Timeline

hokein created this revision.Jul 30 2020, 1:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2020, 1:47 AM
hokein requested review of this revision.Jul 30 2020, 1:47 AM
kadircet added inline comments.Jul 30 2020, 2:10 AM
clang-tools-extra/clangd/XRefs.cpp
428–429

this is the 3rd time we are checking for Sym.Definition, what about moving the check on line 410 to down here? Then it would look something like this:

LocatedSymbol Located;
Located.PreferredDeclaration = DeclLoc;
if (Sym.Definition) {
      auto MaybeDefLoc = indexToLSPLocation(Sym.Definition, MainFilePath);
      if (!MaybeDefLoc) {
        log("locateSymbolNamedTextuallyAt: {0}", MaybeDefLoc.takeError());
        return;
      }
      Located.PreferredDeclaration = *MaybeDefLoc;
      Located.Definition = *MaybeDefLoc;
}
Located.Name = (Sym.Name + Sym.TemplateSpecializationArgs).str();
clang-tools-extra/clangd/unittests/XRefsTests.cpp
285

i am not sure if this change is aligned with the whole idea. we are trying to make the matcher more explicit, so if user wants to assert on the definition being equal to some range I think they should be explicitly specifying it. so I believe this should keep passing llvm::None to underlying matcher.

I know it is likely more work than this version, but otherwise we are not really making it explicit.

hokein updated this revision to Diff 281852.Jul 30 2020, 2:37 AM

address comment.

hokein added inline comments.Jul 30 2020, 2:38 AM
clang-tools-extra/clangd/XRefs.cpp
428–429

good point.

clang-tools-extra/clangd/unittests/XRefsTests.cpp
285

yeah, it seems odd. I think this function is used as a syntax sugar to check LocatedSymbol has the *same* decl/def range, most callers have this assumptions.

options:

  1. keeps it as is, and rename it to something like SymWithSameDeclDefRange (a better name is welcome);
  2. remove it, and use the 3-arg matcher above, callers have to be more verbose (passing decl/def ranges explicitly, even they are the same), and we have 15 references;

1 is my preference, what do you think?

kadircet added inline comments.Jul 30 2020, 3:23 AM
clang-tools-extra/clangd/unittests/XRefsTests.cpp
285

yeah, it seems odd. I think this function is used as a syntax sugar to check LocatedSymbol has the *same* decl/def range, most callers have this assumptions.

my reading of the previous state was:

the real matcher treated llvm::None on the last parameter as a Dont care, it was basically ignoring the definition. hence the two parameter version was used in places where people didn't care about definition range (or even its existence). so this patch is changing the behaviour of this two parameter version from don't care to make sure they are equal, i am not saying that's a bad thing but it just feels like more implicit (which is against the idea of the patch).

keeps it as is, and rename it to something like SymWithSameDeclDefRange (a better name is welcome);

renaming would be one way of dealing with implicitness here. i am just not sure if it is really worth it though. I don't really think the existing call sites had the idea of make sure def and decl range are identical in mind. so I would opt for 2nd option you proposed if that's ok with you.

hokein updated this revision to Diff 281904.Jul 30 2020, 5:57 AM

remove the 2-arg Sym matcher.

kadircet accepted this revision.Jul 30 2020, 7:37 AM

thanks, lgtm!

This revision is now accepted and ready to land.Jul 30 2020, 7:37 AM
This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.