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

Sam McCall via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Dec 11 09:38:14 PST 2017

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


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?

  rCTE Clang Tools Extra


More information about the cfe-commits mailing list