Page MenuHomePhabricator

[clangd] Add xref for macros to FileIndex.
ClosedPublic

Authored by usaxena95 on Dec 12 2019, 3:17 AM.

Details

Summary

Adds macro references to the dynamic index (FileIndex).
Tests added.

Diff Detail

Event Timeline

usaxena95 created this revision.Dec 12 2019, 3:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 12 2019, 3:17 AM

Unit tests: pass. 60496 tests passed, 0 failed and 726 were skipped.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or apply this patch.

Build artifacts: console-log.txt, CMakeCache.txt, clang-format.patch, test-results.xml, diff.json

ilya-biryukov added inline comments.Dec 16 2019, 6:07 AM
clang-tools-extra/clangd/URI.h
115 ↗(On Diff #233561)

This function does a very specialized form of path-to-uri conversion (fallback dirs, traversing schemes, etc.)
I'm afraid that naming it toURI will mean everyone will find and use it without giving it too much thought.

Another concern is layering. URI.h currently does not depend on SourceManager and, arguably, it should stay that way. We define an interface to extend the URI schemes that clangd supports, there's no reason for it to depend on SourceManager.

clang-tools-extra/clangd/index/FileIndex.cpp
88

This is trying to emulate existing logic in SymbolCollector::finish. Is there a way we could share this?
Would avoid creating extra copies of reference slabs and allow to keep the code in one place, rather than scattering it between FileIndex.cpp and SymbolCollector.cpp. Would also allow to keep toURI private, meaning we don't have to worry about naming it and the fact it's exposed in the public interface.

One potential way to do this is to have an alternative version of handleMacroOccurence, which would fill SymbolCollector::MacroRefs directly and call this right after indexTopLevelDecls.
Would that work?

@ilya-biryukov thanks for taking it during my OOO, just a drive-by comment :)

clang-tools-extra/clangd/index/FileIndex.cpp
88

+1 on the concern about performance (this is a hot function, we are paying an extra cost of copying all refs, which should be avoided), and the layering of toURI.

One potential way to do this is to have an alternative version of handleMacroOccurence, which would fill SymbolCollector::MacroRefs directly and call this right after indexTopLevelDecls.
Would that work?

the main problem is that at this point (parsing is finished), preprocessor callback is not available, so we won't see macro references (SymbolCollector::handleMacroOccurence will only receive macro definitions).

I think two options:

  • as mentioned above, add alternative version of handleMacroOccurence in SymbolCollector, calling it before indexTopLevelDecls (because finish is called in indexTopLevelDecls which we have no way to customize);
  • or we could add a finish callback to the SymbolCollector which is called in SymbolCollector::finish, and put this logic to the callback.
ilya-biryukov added inline comments.Dec 17 2019, 6:48 AM
clang-tools-extra/clangd/index/FileIndex.cpp
88

Thanks, good point, in my proposal we should populate macro references before calling indexTopLevelDecls.

I would still suggest going with the first option, it seems simpler to me. But up to you, @usaxena95.

usaxena95 updated this revision to Diff 236037.Fri, Jan 3, 5:52 AM
usaxena95 marked 5 inline comments as done.

Added another version of handleMacroOccurence to handle MacroReferences from main file.

clang-tools-extra/clangd/index/FileIndex.cpp
88

This looks much better now. Thanks for your inputs Haojian and Ilya.
I have added another version of handleMacroOccurence to add the MainFileMacros to the references.

add alternative version of handleMacroOccurence in SymbolCollector, calling it before indexTopLevelDecls

@hokein
SymbolCollector doesn't build the slabs in the finish so I think it is still fine to call it after indexTopLevelDecls. But doing this before finish makes more sense semantically and reasonable.

Unit tests: pass. 60496 tests passed, 0 failed and 726 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

ilya-biryukov added inline comments.Tue, Jan 7, 12:45 AM
clang-tools-extra/clangd/index/SymbolCollector.cpp
354

Does this ever happen?
Maybe assert it's not null instead?

366

Won't we get a dangling pointer inside Refs after leaving this function?
MainFileURI gets deallocated at the end, right?

clang-tools-extra/clangd/index/SymbolCollector.h
108

NIT: could you name it in plural? handleMacros or handleMacroOccurences?

It's different from the other version in this regard, which adds only a single occurrence.
It's also a good practice to avoid adding overloads to virtual/overriden methods, might get confusing at times.

usaxena95 updated this revision to Diff 236541.Tue, Jan 7, 2:54 AM
usaxena95 marked 3 inline comments as done.

Addressed comments.

usaxena95 marked 2 inline comments as done.Tue, Jan 7, 2:55 AM
usaxena95 added inline comments.
clang-tools-extra/clangd/index/SymbolCollector.cpp
366

Yeah I also thought the same when I saw this at other places too. But the Refs.insert(IDToRefs.first, R); actually does a copy of this string and owns the copy.

clang-tools-extra/clangd/index/SymbolCollector.h
108

Renamed to handleMacros to avoid confusion with the virtual method.

ilya-biryukov accepted this revision.Tue, Jan 7, 3:00 AM
ilya-biryukov marked an inline comment as done.

LGTM

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

Ah, makes sense. Thanks for explaining this.

clang-tools-extra/clangd/unittests/FileIndexTests.cpp
368

Why do we need two tests? To test the file URI?
Setting a custom filename ("test.cc" in our case) should be enough for this.

I suggest removing Test2 completely, it does not seem to test anything new and creates noise. Unless there are things I'm missing that it test.

This revision is now accepted and ready to land.Tue, Jan 7, 3:00 AM

Unit tests: pass. 60496 tests passed, 0 failed and 726 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

usaxena95 updated this revision to Diff 236569.Tue, Jan 7, 6:00 AM
usaxena95 marked an inline comment as done.

Removed extra tests.

Unit tests: pass. 60496 tests passed, 0 failed and 726 were skipped.

clang-tidy: pass.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

usaxena95 updated this revision to Diff 236618.Tue, Jan 7, 9:39 AM

Added tests.

Unit tests: pass. 61298 tests passed, 0 failed and 736 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

usaxena95 edited the summary of this revision. (Show Details)Tue, Jan 7, 11:27 PM
This revision was automatically updated to reflect the committed changes.
usaxena95 marked an inline comment as done.Tue, Jan 7, 11:44 PM