[PATCH] D40897: [clangd] Introduce a "Symbol" class.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

\o/



================
Comment at: clangd/index/Index.cpp:34
+
+SymbolSlab::const_iterator SymbolSlab::find(const SymbolID& SymID) const {
+  return Symbols.find(SymID);
----------------
assert frozen? (and in begin())


================
Comment at: clangd/index/Index.h:45
+  SymbolID(llvm::StringRef USR);
+  std::array<uint8_t, 20> HashValue;
+};
----------------
nit: make HashValue private?

provide operator== (and use it from DenseMapInfo).


================
Comment at: clangd/index/Index.h:108
+  static inline clang::clangd::SymbolID getEmptyKey() {
+    return clang::clangd::SymbolID("EMPTYKEY");
+  }
----------------
nit: you may want to memoize this in a local static variable, rather than compute it each time: DenseMap calls it a lot.


================
Comment at: clangd/index/SymbolCollector.cpp:37
+                 << '\n';
+  // Handle symbolic link path cases.
+  // We are trying to get the real file path of the symlink.
----------------
Can you spell out here which symbolic link cases we're handling, and what problems we're trying to avoid?

Offline, we talked about the CWD being a symlink. But this is a different case...


================
Comment at: clangd/index/SymbolCollector.h:35
+
+  StringRef getFilename() const {
+    return Filename;
----------------
What's this for? Seems like we should be able to handle multiple TUs with one collector?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D40897