This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Use expansion location when the ref is inside macros.
ClosedPublic

Authored by hokein on Nov 20 2019, 1:20 AM.

Details

Summary

Previously, xrefs has inconsistent behavior when the reference is inside
macro body:

  • AST-based xrefs (for main file) uses the expansion location;
  • our index uses the spelling location;

This patch makes our index use expansion location, which is consistent with
AST-based xrefs, and kythe as well.

Diff Detail

Event Timeline

hokein created this revision.Nov 20 2019, 1:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 20 2019, 1:20 AM
ilya-biryukov added inline comments.Nov 20 2019, 1:26 AM
clang-tools-extra/clangd/index/SymbolCollector.cpp
280

We're using getSpellingLoc here and getFileLoc later. Why not use getFileLoc everywhere?

Having a variable (similar to the SpellingLoc we had before) and calling getFileLoc only once also seems preferable.

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
655

Could you also check:

  1. Multi-level macro arguments TYPE(TYPE(FOO))
  2. Concatenated tokens as identifiers, e.g. coming from #define CAT(X, Y) X##Y

Build result: pass - 60164 tests passed, 0 failed and 731 were skipped.
Log files: console-log.txt, CMakeCache.txt

hokein updated this revision to Diff 230249.Nov 20 2019, 6:05 AM
hokein marked 3 inline comments as done.

add more tests.

clang-tools-extra/clangd/index/SymbolCollector.cpp
280

We're using getSpellingLoc here and getFileLoc later. Why not use getFileLoc everywhere?

There are two things in SymbolCollector:

  • symbols & ranking signals, we use spelling location for them, the code is part of this, ReferencedDecls is used to calculate the ranking
  • references

this patch only targets the reference part (changing the loc here would break many assumptions I think, and there was a failure test).

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
655

Added tests.

Build result: pass - 60164 tests passed, 0 failed and 731 were skipped.
Log files: console-log.txt, CMakeCache.txt

ilya-biryukov added inline comments.Nov 21 2019, 12:53 AM
clang-tools-extra/clangd/index/SymbolCollector.cpp
280
  • What are the assumptions that it will break?
  • What is rationale for using spelling locations for ranking and file location for references?

It would be nice to have this spelled out somewhere in the code, too. Currently this looks like an accidental inconsistency. Especially given that getFileLoc and getSpellingLoc are often the same.

ilya-biryukov marked an inline comment as done.Nov 21 2019, 12:57 AM
ilya-biryukov added inline comments.
clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
659

Previously we would not report any location at all in that case, right?
Not sure how common this is, hope this won't blow up our index size too much. No need to change anything now, but we should be ready to revert if needed.

Worth putting a comment here that AST-based XRefs behave in the same way. (And maybe adding a test there, if there isn't one already)

hokein updated this revision to Diff 230905.Nov 25 2019, 8:03 AM
hokein marked 2 inline comments as done.

address comments

  • add related test for ast-based xrefs
  • add comments to clarify using different location for symbols/references.
clang-tools-extra/clangd/index/SymbolCollector.cpp
280

Added comments to clarify the difference between references and other fields.

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
659

Yes, I measure the memory usage before vs after, it increased ~5% memory usage.

Build result: pass - 60296 tests passed, 0 failed and 732 were skipped.

Log files: console-log.txt, CMakeCache.txt

ilya-biryukov added inline comments.Nov 26 2019, 1:17 AM
clang-tools-extra/clangd/index/SymbolCollector.cpp
280

The comment still does not explain *why* we do this.
Why not use the same type of locations for everything?

clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
659

On LLVM? This doesn't look too bad, but probably worth mentioning it in the patch description

ilya-biryukov added inline comments.Dec 9 2019, 5:17 AM
clang-tools-extra/clangd/index/SymbolCollector.cpp
280

Update after offline discussion:
there does not seem to be any good reason to use spelling locations and not file locations here. Some reference counts will be affected, but only those that end up being spelled in the macros inside the main file and used outside the main file. Which is very rare and shouldn't affect completion ranking much.

We should definitely try switching to file locations everywhere, but that means doing more changes. Therefore, it's more appropriate to do it in the follow-up change.

This revision is now accepted and ready to land.Dec 9 2019, 5:17 AM
hokein updated this revision to Diff 232853.Dec 9 2019, 7:29 AM

add a fixme.

Build result: pass - 60625 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

This revision was automatically updated to reflect the committed changes.