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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/index/Index.cpp:139
+    std::sort(Occurrences.begin(), Occurrences.end(),
+              [](const SymbolOccurrence &L, const SymbolOccurrence &R) {
+                return L < R;
----------------
NIT: remove the lambda? using `<` is the default.


================
Comment at: clangd/index/Index.cpp:145
+                    [](const SymbolOccurrence &L, const SymbolOccurrence &R) {
+                      return L == R;
+                    }),
----------------
NIT: remove the lambda? Using `==` is the default.


================
Comment at: clangd/index/Index.cpp:158
+raw_ostream &operator<<(raw_ostream &OS, SymbolOccurrenceKind K) {
+  OS << static_cast<unsigned>(K);
+  return OS;
----------------
Is this used for debugging?
In that case maybe consider having a user-readable representation instead of the number?


================
Comment at: clangd/index/Index.h:46
+
+inline bool operator==(const SymbolLocation::Position &L,
+                       const SymbolLocation::Position &R) {
----------------
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.


================
Comment at: clangd/index/Index.h:349
     llvm::DenseMap<SymbolID, size_t> SymbolIndex;
+
+    llvm::DenseMap<SymbolID, std::vector<SymbolOccurrence>> SymbolOccurrences;
----------------
Maybe add a comment or remove the empty line?


================
Comment at: clangd/index/Index.h:350
+
+    llvm::DenseMap<SymbolID, std::vector<SymbolOccurrence>> SymbolOccurrences;
   };
----------------
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.


================
Comment at: clangd/index/SymbolCollector.cpp:272
+    };
+    if (static_cast<unsigned>(Opts.Filter) & Roles) {
+      if (auto ID = getSymbolID(D)) {
----------------
NIT: maybe use early exits and inverted conditions to keep the nesting down?


================
Comment at: clangd/index/SymbolCollector.cpp:321
+
+  const SymbolCollector::Options &CollectorOpts;
+  const SymbolCollector::Options::CollectSymbolOptions &Opts;
----------------
If we any `Options` here, why have an extra `CollectorSymbolOptions`?


================
Comment at: clangd/index/SymbolCollector.h:59
+      // If none, all symbol occurrences will be collected.
+      llvm::Optional<llvm::DenseSet<SymbolID>> IDs = llvm::None;
+    };
----------------
Could you elaborate on what this option will be used for? How do we know in advance which symbols we're interested in?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D50385