[PATCH] D70489: [clangd] Add xref for macro to static index.

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

btw, could you measure the increasing size of the index with this patch?

You could run

  ./bin/clangd-indexer -format=binary -executor=all-TUs . > static-index.idx
  ./bin/dexp static-index.idx
  # you will see the memory usage.



================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:532
+      const IdentifierInfo *II = MacroRef.first;
+      if (const auto *MI = PP->getMacroDefinition(II).getMacroInfo())
+        if (auto ID = getSymbolID(II->getName(), MI, PP->getSourceManager())) {
----------------
usaxena95 wrote:
> hokein wrote:
> > I'm curious of the behavior `getMacroDefinition` -- from its implementation, if `II` doesn't have macro definition, it just returns empty.
> > 
> > could you check whether it works at the below case (or even with a same macro name defined/undef multiple times)?
> > 
> > ```
> > #define abc 1
> > #undef abc
> > 
> > // not at the EOF, I guess, II for `abc` doesn't have macro definition at this point because it has been undef?
> > ```
> I had a FIXME in the tests for this case. I see why there was no macro definition at that point. I guess it would be better to keep the symbol id  instead of the II.
sorry, I didn't see a FIXME in the test, am I missing anything? Maybe move the FIXME to here?


================
Comment at: clang-tools-extra/clangd/index/SymbolCollector.h:76
     /// macro even if this is true.
     bool CollectMacro = false;
     /// Collect symbols local to main-files, such as static functions
----------------
usaxena95 wrote:
> hokein wrote:
> > This option is for macro symbols, I'd not use it for collecting macro references. I think the whether to collect macro references is judged by `RefFilter` and `RefsInHeaders`. 
> I see. `RefFilter` sounds right but I am don't think `RefsInHeaders` is the correct one. It just tells whether to collect references from the included header (i.e. outside mainfile) or not.
> I think it would be better to have a separate flag for macro references in such case.
> WDYT ?
I don't think we need an extra flag merely for macro references. macro references should be treated in the same way of decl references.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70489/new/

https://reviews.llvm.org/D70489