ilya-biryukov accepted this revision. ilya-biryukov added a comment. This revision is now accepted and ready to land.
LGTM ================ 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, ---------------- ioeric wrote: > 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. Agreed, let's see if anyone complains instead of assuming backwards-compatibility is a requirement. ================ Comment at: unittests/Index/IndexTests.cpp:61 + S.Roles = Roles; + if (MI) + S.Info = getSymbolInfoForMacro(*MI); ---------------- ioeric wrote: > 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. Ah, makes sense, thanks. Any reason to not report those? It looks like a valuable information (e.g. to avoid showing undefined macros in code completion). 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
