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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Overall LG, see the major comment about running without the preprocessor.



================
Comment at: include/clang/Index/IndexingAction.h:44
   bool IndexImplicitInstantiation = false;
+  bool IndexMacrosInPreprocessor = false;
 };
----------------
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`.


================
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,
----------------
This does not yet index macro references, maybe keep a mention of this in a comment?


================
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,
----------------
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?


================
Comment at: unittests/Index/IndexTests.cpp:30
+  std::string QName;
+  SymbolRoleSet Roles;
+  SymbolInfo Info;
----------------
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.


================
Comment at: unittests/Index/IndexTests.cpp:36
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const TestSymbol &S) {
+  return OS << S.QName;
+}
----------------
If we decide to keep `Roles` and `Info`, maybe consider outputting them here?
To make sure we don't miss them later in case some tests fail.


================
Comment at: unittests/Index/IndexTests.cpp:61
+    S.Roles = Roles;
+    if (MI)
+      S.Info = getSymbolInfoForMacro(*MI);
----------------
Can this actually happen? It seems weird to have a macro occurrence without a `MacroInfo`.
Maybe try asserting that MI is non-null instead?


Repository:
  rC Clang

https://reviews.llvm.org/D52098