Page MenuHomePhabricator

[clangd] Build in-memory index on symbols in files.
ClosedPublic

Authored by ioeric on Dec 15 2017, 12:45 AM.

Event Timeline

ioeric created this revision.Dec 15 2017, 12:45 AM
sammccall added inline comments.Dec 15 2017, 1:51 AM
clangd/index/FileMemIndexManager.h
23 ↗(On Diff #127075)

Discussed offline a bit:

  • FileIndex or PerFileIndex is a great name for this, if it implements Index. So I think it should (and forward to the MemIndex that it owns).
  • FileSymbols is mostly useful to implement this, so we could put them in the same header FileIndexand add a comment to that effect.
  • What this means for unittest organization - up to you
unittests/clangd/IndexTests.cpp
120 ↗(On Diff #127075)

Can you make this build() returning a ParsedAST instead?

It adds a little duplication to the callsite:

M.update("f1", build("f1", "...code..."));

but it makes this a much more "pure" function, and a good candidate for pulling out into a helper for other tests.

ioeric updated this revision to Diff 127086.Dec 15 2017, 2:49 AM
  • Address review comments. Merge FileSymbols and tests into FileIndex.
ioeric updated this revision to Diff 127087.Dec 15 2017, 2:51 AM
  • Minor cleanup
sammccall accepted this revision.Dec 15 2017, 3:17 AM

I don't see FileSymbols.{h,cpp} being deleted, but maybe I'm bad at reading diffs.

clangd/index/FileIndex.h
8

Add a file-comment about what this is and how it's going to be used?
At the moment almost all the comments are on FileSymbols and implementation-focused, while FileIndex is the interesting class.

57

override

This revision is now accepted and ready to land.Dec 15 2017, 3:17 AM
ioeric updated this revision to Diff 127098.Dec 15 2017, 4:21 AM
ioeric marked 2 inline comments as done.
  • Removed unused files and addressed comments.

I don't see FileSymbols.{h,cpp} being deleted, but maybe I'm bad at reading diffs.

No, I'm bad at creating diffs...

This revision was automatically updated to reflect the committed changes.