[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clangd/index/Index.h:46
+
+inline bool operator==(const SymbolLocation::Position &L,
+                       const SymbolLocation::Position &R) {
----------------
ilya-biryukov wrote:
> NIT: having friend decls inside the classes themselves might prove to be more readable.
> Not opposed to the current one too, feel free to ignore.
These operator implementations seem not as much interesting as members in the structure, putting them to the structure probably adds some noise to readers.




================
Comment at: clangd/index/Index.h:350
+
+    llvm::DenseMap<SymbolID, std::vector<SymbolOccurrence>> SymbolOccurrences;
   };
----------------
ilya-biryukov wrote:
> Any store occurences in a file-centric manner?
> E.g. 
> ```
> /// Occurences inside a single file.
> class FileOccurences {
>   StringRef File;
>   vector<pair<Point, OccurenceKind>> Locations;
> };
> 
> // ....
> DenseMap<SymbolID, vector<FileOccurences>>  SymbolOccurences;
> ```
> 
> As discussed previously, this representation is better suited for both merging and serialization.
The file-centric manner doesn't seem to suite our current model:  whenever we update the index for the main AST, we just replace the symbol slab with the new one; and for index merging, we only use the index `findOccurrences` interfaces.

It would save some memory usage of `StringRef` File, but AFAIK, the memory usage of current model is relatively small (comparing with the SymbolSlab for code completion) since we only store occurrences in main file (~50KB for `CodeComplete.cpp`).

I'd leave it as it is now, and we could revisit it later.


================
Comment at: clangd/index/SymbolCollector.h:59
+      // If none, all symbol occurrences will be collected.
+      llvm::Optional<llvm::DenseSet<SymbolID>> IDs = llvm::None;
+    };
----------------
ilya-biryukov wrote:
> Could you elaborate on what this option will be used for? How do we know in advance which symbols we're interested in?
This is used for finding references in the AST as a part of the xref implementation, basically the workflow would be:

1. find SymbolIDs of the symbol under the cursor, using `DeclarationAndMacrosFinder`
2. run symbol collector to find all occurrences in the main AST with all SymbolIDs in #1
3. query the index, to get more occurrences
4. merge them  


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385