[PATCH] D49028: [clangd] Support indexing MACROs.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clangd/index/SymbolCollector.cpp:309
 
+  llvm::SmallString<128> USR;
+  if (index::generateUSRForDecl(ND, USR))
----------------
why this change? I think this makes us run generateUSR much more often (once per non-unique reference in *any* file, vs unique refeneces in main file), keeping track of a few extra referenced symbols by pointer should be much cheaper


================
Comment at: clangd/index/SymbolCollector.cpp:349
+                                           SourceLocation Loc) {
+  assert(PP.get() && "Preprocessor must be set.");
+  if (!Opts.CollectMacro)
----------------
drop the message unless it has something new to say


================
Comment at: clangd/index/SymbolCollector.cpp:349
+                                           SourceLocation Loc) {
+  assert(PP.get() && "Preprocessor must be set.");
+  if (!Opts.CollectMacro)
----------------
sammccall wrote:
> drop the message unless it has something new to say
(why) do we require PP to be set if CollectMacro is false?


================
Comment at: clangd/index/SymbolCollector.cpp:356
+    return true;
+  // Builtin macro should already be available in sema.
+  if (MI.isUsedForHeaderGuard() || MI.isBuiltinMacro())
----------------
not sure what this means. Are you talking about code completion? This isn't CodeComplete :-)

Header guard macros are clearly not useful in the index, but are probably worth a comment. Builtin macros don't have useful locations, and also aren't needed for code completion as you say.


================
Comment at: clangd/index/SymbolCollector.cpp:360
+
+  llvm::SmallString<128> USR;
+  if (index::generateUSRForMacro(Name.getName(), MI.getDefinitionLoc(), SM,
----------------
as above, can we avoid generating the USR for every reference?
The macro name or hash thereof should be enough to identify it, right?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D49028