[PATCH] D77732: [clangd] Shard preamble symbols in dynamic index

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/index/FileIndex.h:165
+  /// Returns absolute paths for all files that has a shard.
+  std::vector<PathRef> getAllFiles() const;
+
----------------
sammccall wrote:
> I do find it a little weird not to expose the map-structure of the vast majority of the data here.
> 
> What steers you away from just making this a function
> `StringMap<X> shardIndexToFile(IndexFileIn, PathRef)`
> where X could be `File` with a method to obtain the data, or the more anonymous `unique_function<IndexFileIn()>`?
because the map structure is just pointers into the `IndexFileIn`, I wanted to make sure those pointers do not outlive the IndexFileIn.


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:286
 
+  // ND is the canonical (i.e. first) declaration. If it's in the main file
+  // (which is not a header), then no public declaration was visible, so assume
----------------
sammccall wrote:
> kadircet wrote:
> > put this before `processRelations` to ensure `Subject` of a relation is a symbol we are going to collect.
> The code around this seems unchanged - how does this relate to the rest of the patch?
> (Other than I guess you've been inspecting the data and finding bogus shards :-)
> 
> This is bit subtle, and hard to reason about since we only have one example. Indeed dangling relations are pointless. However:
>  - the direction of the relation is fairly arbitrary. Should we be checking shouldCollectSymbol on the object of the relation too?
>  - shouldCollectSymbols depends somewhat on the file context, and also the indexer options. In principle the subject could be indexable elsewhere but not here. Or this index could see the relation and another could see the symbol itself. Can't think of compelling examples though, just hypotheticals.
> 
> I think probably we should give this a comment (about not wanting relations or refs either), add filtering on the relation objects, and add a test (I think that should be pretty easy with inheriting from a main-file-only class?).
> Maybe it's a separate patch?
> The code around this seems unchanged - how does this relate to the rest of the patch?
> (Other than I guess you've been inspecting the data and finding bogus shards :-)

Selection of file declaring the Subject seemed arbitrary, and turned out to be broken. We were not hitting this case before because background index was performing pre filtering and skipping any relation that mapped to a "non interesting" file, hence we didn't notice it was broken. Now it become obvious as we assert on the file containing the subject symbol.

> the direction of the relation is fairly arbitrary. Should we be checking shouldCollectSymbol on the object of the relation too?
> shouldCollectSymbols depends somewhat on the file context, and also the indexer options. In principle the subject could be indexable elsewhere but not here. Or this index could see the relation and another could see the symbol itself. Can't think of compelling examples though, just hypotheticals.
> I think probably we should give this a comment (about not wanting relations or refs either), add filtering on the relation objects, and add a test (I think that should be pretty easy with inheriting from a main-file-only class?).
> Maybe it's a separate patch?

I also wanted to do this but it got hairy and as you also have suggested I decided to have a separate patch for this. Because there were other problematic stuff, for example `IndexerOpts::CollectMainFileSymbols` is always true.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77732/new/

https://reviews.llvm.org/D77732