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

[ioeric]

Summary

Also added unit tests for the index library; lit+c-index-test is painful...

[ioeric]

• remove debug message.

[ioeric]

• another cleanup...

[ilya-biryukov]

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

include/clang/Index/IndexingAction.h

44 ↗

(On Diff #165516)

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.

53 ↗

(On Diff #165516)

This does not yet index macro references, maybe keep a mention of this in a comment?

59 ↗

(On Diff #165516)

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?

unittests/Index/IndexTests.cpp

30 ↗

(On Diff #165516)

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.

36 ↗

(On Diff #165516)

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.

61 ↗

(On Diff #165516)

Can this actually happen? It seems weird to have a macro occurrence without a MacroInfo.
Maybe try asserting that MI is non-null instead?

[ioeric]

• addressed review comments.

include/clang/Index/IndexingAction.h

44 ↗

(On Diff #165516)

Done. Added a comment about this. I think InPreprocessor also indicates this to some extend.

53 ↗

(On Diff #165516)

Covered this in the IndexMacrosInPreprocessor option, which seems to be a better place to document this.

59 ↗

(On Diff #165516)

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.

unittests/Index/IndexTests.cpp

30 ↗

(On Diff #165516)

Sure.

61 ↗

(On Diff #165516)

this can happen for macros that are #undefined. Not relevant anymore.

[ilya-biryukov]

LGTM

include/clang/Index/IndexingAction.h

59 ↗

(On Diff #165516)

Agreed, let's see if anyone complains instead of assuming backwards-compatibility is a requirement.

unittests/Index/IndexTests.cpp

61 ↗

(On Diff #165516)

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).

[ioeric]

unittests/Index/IndexTests.cpp

61 ↗

(On Diff #165516)

I don't think this is a bug e.g. SemaCodeCompletion can intentionally show undefined macros in some contexts. It's up to the index consumer implementations how to handle undefinitions.

cfe/trunk/include/clang/Index/IndexingAction.h

 //===--- IndexingAction.h - Frontend index action ---------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_INDEX_INDEXINGACTION_H
 #define LLVM_CLANG_INDEX_INDEXINGACTION_H
 
 #include "clang/Basic/LLVM.h"
 #include "clang/Lex/PPCallbacks.h"
+#include "clang/Lex/Preprocessor.h"
 #include "llvm/ADT/ArrayRef.h"
 #include <memory>
 
 namespace clang {
   class ASTContext;
   class ASTReader;
   class ASTUnit;
   class Decl;
enum class SystemSymbolFilterKind {
     DeclarationsOnly,
     All,
   };
 
   SystemSymbolFilterKind SystemSymbolFilter
     = SystemSymbolFilterKind::DeclarationsOnly;
   bool IndexFunctionLocals = false;
   bool IndexImplicitInstantiation = false;
+  // Whether to index macro definitions in the Preprocesor when preprocessor
+  // callback is not available (e.g. after parsing has finished). Note that
+  // macro references are not available in Proprocessor.
+  bool IndexMacrosInPreprocessor = false;
 };
 
 /// Creates a frontend action that indexes all symbols (macros and AST decls).
 /// \param WrappedAction another frontend action to wrap over or null.
 std::unique_ptr<FrontendAction>
 createIndexingAction(std::shared_ptr<IndexDataConsumer> DataConsumer,
                      IndexingOptions Opts,
                      std::unique_ptr<FrontendAction> WrappedAction);
 
 /// Recursively indexes all decls in the AST.
-/// Note that this does not index macros.
 void indexASTUnit(ASTUnit &Unit, IndexDataConsumer &DataConsumer,
                   IndexingOptions Opts);
 
 /// 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,
                         IndexDataConsumer &DataConsumer, IndexingOptions Opts);
 
 /// Creates a PPCallbacks that indexes macros and feeds macros to \p Consumer.
 /// The caller is responsible for calling `Consumer.setPreprocessor()`.
 std::unique_ptr<PPCallbacks> indexMacrosCallback(IndexDataConsumer &Consumer,
                                                  IndexingOptions Opts);
 
 /// Recursively indexes all top-level decls in the module.
-/// FIXME: make this index macros as well.
 void indexModuleFile(serialization::ModuleFile &Mod, ASTReader &Reader,
                      IndexDataConsumer &DataConsumer, IndexingOptions Opts);
 
 } // namespace index
 } // namespace clang
 
 #endif

cfe/trunk/lib/Index/IndexingAction.cpp

static bool topLevelDeclVisitor(void *context, const Decl *D) {
   IndexingContext &IndexCtx = *static_cast<IndexingContext*>(context);
   return IndexCtx.indexTopLevelDecl(D);
 }
 
 static void indexTranslationUnit(ASTUnit &Unit, IndexingContext &IndexCtx) {
   Unit.visitLocalTopLevelDecls(&IndexCtx, topLevelDeclVisitor);
 }
 
+static void indexPreprocessorMacros(const Preprocessor &PP,
+                                    IndexDataConsumer &DataConsumer) {
+  for (const auto &M : PP.macros())
+    if (MacroDirective *MD = M.second.getLatest())
+      DataConsumer.handleMacroOccurence(
+          M.first, MD->getMacroInfo(),
+          static_cast<unsigned>(index::SymbolRole::Definition),
+          MD->getLocation());
+}
+
 void index::indexASTUnit(ASTUnit &Unit, IndexDataConsumer &DataConsumer,
                          IndexingOptions Opts) {
   IndexingContext IndexCtx(Opts, DataConsumer);
   IndexCtx.setASTContext(Unit.getASTContext());
   DataConsumer.initialize(Unit.getASTContext());
   DataConsumer.setPreprocessor(Unit.getPreprocessorPtr());
+
+  if (Opts.IndexMacrosInPreprocessor)
+    indexPreprocessorMacros(Unit.getPreprocessor(), DataConsumer);
   indexTranslationUnit(Unit, IndexCtx);
   DataConsumer.finish();
 }
 
-void index::indexTopLevelDecls(ASTContext &Ctx, ArrayRef<const Decl *> Decls,
+void index::indexTopLevelDecls(ASTContext &Ctx, Preprocessor &PP,
+                               ArrayRef<const Decl *> Decls,
                                IndexDataConsumer &DataConsumer,
                                IndexingOptions Opts) {
   IndexingContext IndexCtx(Opts, DataConsumer);
   IndexCtx.setASTContext(Ctx);
 
   DataConsumer.initialize(Ctx);
+
+  if (Opts.IndexMacrosInPreprocessor)
+    indexPreprocessorMacros(PP, DataConsumer);
+
   for (const Decl *D : Decls)
     IndexCtx.indexTopLevelDecl(D);
   DataConsumer.finish();
 }
 
 std::unique_ptr<PPCallbacks>
 index::indexMacrosCallback(IndexDataConsumer &Consumer, IndexingOptions Opts) {
   return llvm::make_unique<IndexPPCallbacks>(
       std::make_shared<IndexingContext>(Opts, Consumer));
 }
 
 void index::indexModuleFile(serialization::ModuleFile &Mod, ASTReader &Reader,
                             IndexDataConsumer &DataConsumer,
                             IndexingOptions Opts) {
   ASTContext &Ctx = Reader.getContext();
   IndexingContext IndexCtx(Opts, DataConsumer);
   IndexCtx.setASTContext(Ctx);
   DataConsumer.initialize(Ctx);
 
+  if (Opts.IndexMacrosInPreprocessor)
+    indexPreprocessorMacros(Reader.getPreprocessor(), DataConsumer);
+
   for (const Decl *D : Reader.getModuleFileLevelDecls(Mod)) {
     IndexCtx.indexTopLevelDecl(D);
   }
   DataConsumer.finish();
 }

cfe/trunk/unittests/CMakeLists.txt

 add_subdirectory(Sema)
 add_subdirectory(CodeGen)
 # FIXME: libclang unit tests are disabled on Windows due
 # to failures, mostly in libclang.VirtualFileOverlay_*.
 if(NOT WIN32 AND CLANG_TOOL_LIBCLANG_BUILD) 
   add_subdirectory(libclang)
 endif()
 add_subdirectory(Rename)
+add_subdirectory(Index)

cfe/trunk/unittests/Index/CMakeLists.txt

+set(LLVM_LINK_COMPONENTS
+  ${LLVM_TARGETS_TO_BUILD}
+  Support
+  )
+
+add_clang_unittest(IndexTests
+  IndexTests.cpp
+  )
+
+target_link_libraries(IndexTests
+  PRIVATE
+  clangAST
+  clangBasic
+  clangFrontend
+  clangIndex
+  clangLex
+  clangTooling
+  )

cfe/trunk/unittests/Index/IndexTests.cpp

+//===--- IndexTests.cpp - Test indexing actions -----------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/Decl.h"
+#include "clang/Basic/VirtualFileSystem.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Index/IndexDataConsumer.h"
+#include "clang/Index/IndexSymbol.h"
+#include "clang/Index/IndexingAction.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/StringRef.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include <memory>
+
+namespace clang {
+namespace index {
+
+struct TestSymbol {
+  std::string QName;
+  // FIXME: add more information.
+};
+
+llvm::raw_ostream &operator<<(llvm::raw_ostream &OS, const TestSymbol &S) {
+  return OS << S.QName;
+}
+
+namespace {
+class Indexer : public IndexDataConsumer {
+public:
+  bool handleDeclOccurence(const Decl *D, SymbolRoleSet Roles,
+                           ArrayRef<SymbolRelation>, SourceLocation,
+                           ASTNodeInfo) override {
+    const auto *ND = llvm::dyn_cast<NamedDecl>(D);
+    if (!ND)
+      return true;
+    TestSymbol S;
+    S.QName = ND->getQualifiedNameAsString();
+    Symbols.push_back(std::move(S));
+    return true;
+  }
+
+  bool handleMacroOccurence(const IdentifierInfo *Name, const MacroInfo *,
+                            SymbolRoleSet, SourceLocation) override {
+    TestSymbol S;
+    S.QName = Name->getName();
+    Symbols.push_back(std::move(S));
+    return true;
+  }
+
+  std::vector<TestSymbol> Symbols;
+};
+
+class IndexAction : public ASTFrontendAction {
+public:
+  IndexAction(std::shared_ptr<Indexer> Index,
+              IndexingOptions Opts = IndexingOptions())
+      : Index(std::move(Index)), Opts(Opts) {}
+
+protected:
+  std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
+                                                 StringRef InFile) override {
+    class Consumer : public ASTConsumer {
+      std::shared_ptr<Indexer> Index;
+      std::shared_ptr<Preprocessor> PP;
+      IndexingOptions Opts;
+
+    public:
+      Consumer(std::shared_ptr<Indexer> Index, std::shared_ptr<Preprocessor> PP,
+               IndexingOptions Opts)
+          : Index(std::move(Index)), PP(std::move(PP)), Opts(Opts) {}
+
+      void HandleTranslationUnit(ASTContext &Ctx) override {
+        std::vector<Decl *> DeclsToIndex(
+            Ctx.getTranslationUnitDecl()->decls().begin(),
+            Ctx.getTranslationUnitDecl()->decls().end());
+        indexTopLevelDecls(Ctx, *PP, DeclsToIndex, *Index, Opts);
+      }
+    };
+    return llvm::make_unique<Consumer>(Index, CI.getPreprocessorPtr(), Opts);
+  }
+
+private:
+  std::shared_ptr<Indexer> Index;
+  IndexingOptions Opts;
+};
+
+using testing::Contains;
+using testing::Not;
+using testing::UnorderedElementsAre;
+
+MATCHER_P(QName, Name, "") { return arg.QName == Name; }
+
+TEST(IndexTest, Simple) {
+  auto Index = std::make_shared<Indexer>();
+  tooling::runToolOnCode(new IndexAction(Index), "class X {}; void f() {}");
+  EXPECT_THAT(Index->Symbols, UnorderedElementsAre(QName("X"), QName("f")));
+}
+
+TEST(IndexTest, IndexPreprocessorMacros) {
+  std::string Code = "#define INDEX_MAC 1";
+  auto Index = std::make_shared<Indexer>();
+  IndexingOptions Opts;
+  Opts.IndexMacrosInPreprocessor = true;
+  tooling::runToolOnCode(new IndexAction(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols, Contains(QName("INDEX_MAC")));
+
+  Opts.IndexMacrosInPreprocessor = false;
+  Index->Symbols.clear();
+  tooling::runToolOnCode(new IndexAction(Index, Opts), Code);
+  EXPECT_THAT(Index->Symbols, UnorderedElementsAre());
+}
+
+} // namespace
+} // namespace index
+} // namespace clang