[PATCH] D52098: [Index] Add an option to collect macros from preprocesor.

[Eric Liu via Phabricator via cfe-commits]

ioeric added inline comments.


================
Comment at: include/clang/Index/IndexingAction.h:44
   bool IndexImplicitInstantiation = false;
+  bool IndexMacrosInPreprocessor = false;
 };
----------------
ilya-biryukov wrote:
> Maybe add a comment or change a name to indicate that this currently only indexes macro definitions?
> Macro usages are still only indexed in `createIndexingAction`.
Done. Added a comment about this. I think `InPreprocessor` also indicates this to some extend.


================
Comment at: include/clang/Index/IndexingAction.h:53
 /// Recursively indexes all decls in the AST.
-/// Note that this does not index macros.
 void indexASTUnit(ASTUnit &Unit, IndexDataConsumer &DataConsumer,
----------------
ilya-biryukov wrote:
> This does not yet index macro references, maybe keep a mention of this in a comment?
Covered this in the `IndexMacrosInPreprocessor` option, which seems to be a better place to document this. 


================
Comment at: include/clang/Index/IndexingAction.h:59
 /// Recursively indexes \p Decls.
-/// Note that this does not index macros.
-void indexTopLevelDecls(ASTContext &Ctx, ArrayRef<const Decl *> Decls,
+void indexTopLevelDecls(ASTContext &Ctx, Preprocessor &PP,
+                        ArrayRef<const Decl *> Decls,
----------------
ilya-biryukov wrote:
> It means we won't have the API to index AST without the preprocessor.
> I don't have a strong opinion on whether this change is fine, our usages look ok, but not sure if someone has a use-case that might break.
> 
> We could take a slightly more backwards-compatible approach, add an overload without the preprocessor and assert that the `IndexMacrosInPreprocessor` option is set to false.
> Not worth the trouble if all the clients want macros, though. WDYT?
yeah, not sure if it's worth the trouble. In theory, a PP should always be available when AST is available (I hope the index library could enforce somehow). And having two overloads with slightly different behaviors seems worse than unknown backward compatibility.


================
Comment at: unittests/Index/IndexTests.cpp:30
+  std::string QName;
+  SymbolRoleSet Roles;
+  SymbolInfo Info;
----------------
ilya-biryukov wrote:
> Roles and Info are neither tested nor output currently. Consider simplifying the test code by collecting only qual names and using strings everywhere.
> More info could always be added when needed. 
> 
> Feel free to ignore, e.g. if you feel we need this for future revisions.
Sure.


================
Comment at: unittests/Index/IndexTests.cpp:61
+    S.Roles = Roles;
+    if (MI)
+      S.Info = getSymbolInfoForMacro(*MI);
----------------
ilya-biryukov wrote:
> Can this actually happen? It seems weird to have a macro occurrence without a `MacroInfo`.
> Maybe try asserting that MI is non-null instead?
this can happen for macros that are #undefined. Not relevant anymore.  


Repository:
  rC Clang

https://reviews.llvm.org/D52098