[PATCH] D48961: [Index] Add indexing support for MACROs.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I worry this is a trap: the indexing infrastructure here is designed so you can run it as a frontendaction, on an ASTUnit, or by passing a set of top level decls.
However the macro functionality necessarily only works when running as a frontend action, so the same consumer would have different semantics when using these functions.
Moreover, clangd does call indexTopLevelDecls(), so this API might actually be awkward for us to use.

Alternatives would be:

- write lots of loud documentation and hope people read it
- punt on generalization and just do what we need in clangd with PPCallbacks directly
- offer a peer API here for consuming macros, and have the indexTopLevelDecls() etc only take the symbol consumer, createIndexingAction() would create both (and you could have a createIndexingPPCallbacks() that takes only the macro consumer).

WDYT?



================
Comment at: include/clang/Index/IndexDataConsumer.h:50
+                                    const MacroInfo &MI, SymbolRoleSet Roles,
+                                    SourceLocation Loc, bool Undefined = false);
 
----------------
I know this file isn't heavy on documentation, but I think `Undefined` needs some explanation/example where it might be true. And consider flipping to remove the negation.

Why a default parameter? Virtual + default is slightly confusing, and the #callers here should be few I think.


================
Comment at: include/clang/Index/IndexSymbol.h:138
 
+SymbolInfo getSymbolInfoForMacro();
+
----------------
this should take an arg - maybe macroinfo?
There are potentially subkinds here (function-like macros vs object-like ones) and it'll be harder to update callsites once they exist :-)


Repository:
  rC Clang

https://reviews.llvm.org/D48961