This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add an option to exclude symbols outside of project root from index
AbandonedPublic

Authored by kbobyrev on Jul 28 2020, 4:38 PM.

Details

Reviewers
hokein
Summary

This is useful for the remote index: system libraries and external dependencies
would still have a different location on local and remote machine, so they
should be simply excluded.

Diff Detail

Event Timeline

kbobyrev created this revision.Jul 28 2020, 4:38 PM
kbobyrev requested review of this revision.Jul 28 2020, 4:38 PM
hokein added inline comments.Jul 30 2020, 2:18 AM
clang-tools-extra/clangd/indexer/IndexerMain.cpp
112

I think we could reuse the FileShardedIndex to do the filtering job, rather than implementing a new one.

kbobyrev planned changes to this revision.Jul 30 2020, 2:36 AM
kbobyrev added inline comments.
clang-tools-extra/clangd/indexer/IndexerMain.cpp
112

Ah, this is interesting, I haven't seen FileShardedIndex before, let me take a look at its API, it looks like a convenient piece of infrastructure for implementing this!

kbobyrev updated this revision to Diff 282147.Jul 31 2020, 12:24 AM

Switch to SymbolCollector::Options::FileFilter that is even more
straightforward.

kbobyrev marked an inline comment as done.Jul 31 2020, 12:24 AM
kbobyrev edited the summary of this revision. (Show Details)
hokein added inline comments.Jul 31 2020, 6:11 AM
clang-tools-extra/clangd/indexer/IndexerMain.cpp
73

FileFilter seems more promising, but there are some FIXMEs ("use the result to filter out symbols.") in the SymbolCollector.cpp` -- looks like the SymbolCollector doesn't support this yet, sigh...

kbobyrev added inline comments.Jul 31 2020, 10:27 AM
clang-tools-extra/clangd/indexer/IndexerMain.cpp
73

Hmm, you are right. However, I tried running the indxer with and without new option and the number of symbols & references was different. Hmm, this is interesting, I wonder why this happened.

Yeah, I think I should update the code to make sure everything works. Thanks for noticing it!

kbobyrev planned changes to this revision.Jul 31 2020, 10:28 AM

Need tot resolve FIXMEs around shouldIndexFile().

kbobyrev requested review of this revision.Aug 6 2020, 5:16 AM

Even despite FileFilter not being fully implemented yet (an issue for a separate patch) I think this change should still be correct and is probably OK to land, WDYT @hokein?

hokein added a comment.Aug 6 2020, 7:51 AM

Even despite FileFilter not being fully implemented yet (an issue for a separate patch) I think this change should still be correct and is probably OK to land, WDYT @hokein?

Yes, personally no objection on landing this, but landing it doesn't seem to help much for your remote-index case (STL symbols are not filtered out)?

Even despite FileFilter not being fully implemented yet (an issue for a separate patch) I think this change should still be correct and is probably OK to land, WDYT @hokein?

Yes, personally no objection on landing this, but landing it doesn't seem to help much for your remote-index case (STL symbols are not filtered out)?

True, it doesn't filter _all_ of them, but it partially filters some symbols which is still a win I guess :) My rationale is probably that having this blocked on more implementation details would be more reasonable if there was no implementation at all but since it still filters out some symbols this should probably be fine.

hokein added a comment.Aug 6 2020, 8:24 AM

Even despite FileFilter not being fully implemented yet (an issue for a separate patch) I think this change should still be correct and is probably OK to land, WDYT @hokein?

Yes, personally no objection on landing this, but landing it doesn't seem to help much for your remote-index case (STL symbols are not filtered out)?

True, it doesn't filter _all_ of them, but it partially filters some symbols which is still a win I guess :) My rationale is probably that having this blocked on more implementation details would be more reasonable if there was no implementation at all but since it still filters out some symbols this should probably be fine.

It just reduces symptoms, not solving the problem. I think remote-index is also blocked the FileFiltering stuff (we can't launch remote-index without fixing this issue entirely).

As you may have noticed implementing FileFiltering is tricky, I think the indexer here is a good opportunity to test/verify your implementation (comparing the index data before vs after), so my preference would be to implement the FileFilter, test it with this patch together, then finally land this patch after making sure everything works.

kbobyrev planned changes to this revision.Aug 6 2020, 8:49 AM

Even despite FileFilter not being fully implemented yet (an issue for a separate patch) I think this change should still be correct and is probably OK to land, WDYT @hokein?

Yes, personally no objection on landing this, but landing it doesn't seem to help much for your remote-index case (STL symbols are not filtered out)?

True, it doesn't filter _all_ of them, but it partially filters some symbols which is still a win I guess :) My rationale is probably that having this blocked on more implementation details would be more reasonable if there was no implementation at all but since it still filters out some symbols this should probably be fine.

It just reduces symptoms, not solving the problem. I think remote-index is also blocked the FileFiltering stuff (we can't launch remote-index without fixing this issue entirely).

As you may have noticed implementing FileFiltering is tricky, I think the indexer here is a good opportunity to test/verify your implementation (comparing the index data before vs after), so my preference would be to implement the FileFilter, test it with this patch together, then finally land this patch after making sure everything works.

Fair enough, this is a viable strategy.

kbobyrev abandoned this revision.May 19 2022, 5:51 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 19 2022, 5:51 AM