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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
