Moved the IndexDataConsumer::finish call into the IndexASTConsumer from IndexAction

[gribozavr]

Summary

Doing so removes the last reason to expose a FrontendAction from
libIndex.

[ilya-biryukov]

clang/lib/Index/IndexingAction.cpp

77

The fact that we call addPPCallbacks after creating ASTContext is a bit concerning.

This wouldn't probably cause any problems in practice, but generally changing preprocessor by the time ASTContext is already created seems dangerous (what if something there ever starts depending on the state of the preprocessor?)

I think this concern is easily elevated if we change Preprocessor and call addPPCallbacks on CreateASTConsumer(CompilerInvocation& CI). That would mean we have a separate function that sets up the preprocessor and creates IndexASTConsumer and it should be exposed to the clients.

Have you considered this approach? Would it work?

[gribozavr]

clang/lib/Index/IndexingAction.cpp

77

It does feel a bit weird, but we shouldn't have started parsing before calling Initialize on the ASTConsumer. Therefore I agree with you that it won't cause problems in practice.

Calling addPPCallbacks in FrontendAction is against the goal of this patch set. The goal is to encapsulate as much as possible in the IndexASTConsumer, because they compose well, unlike FrontendActions. Therefore, requiring customization in FrontendAction is not possible.

[jkorous]

clang/lib/Index/IndexingAction.cpp

77

We could also move the preprocessor setup to constructor and check/add asserts that ASTContext has been provided to IndexingContext and IndexDataConsumer implementations.

[ilya-biryukov]

LGTM

clang/lib/Index/IndexingAction.cpp

77

LG, let's keep it like this and hope this won't break in the future too.
And if it will, we would probably be able to fix this by setting up PPCallbacks at the time when we create the ASTConsumer, i.e.

std::unique_ptr<ASTConsumer> createIndexASTConsumer(CompilerInvocation &CI) {
  CI.getPreprocessor().addPPCallback();
  return llvm::make_unique<IndexASTConsumer>(...);
}

That would reduce the gap between FrontendAction::BeginSourceFile and addPPCallbacks to just a few instructions, and the chances of breaking stuff would be even less.

[gribozavr]

Committed as r370336.

clang/lib/Index/IndexingAction.cpp

 #include "llvm/ADT/STLExtras.h"
 #include <memory>
 
 using namespace clang;
 using namespace clang::index;
 
 namespace {
 
-class IndexASTConsumer : public ASTConsumer {
-  std::shared_ptr<Preprocessor> PP;
-  std::shared_ptr<IndexingContext> IndexCtx;
-
-public:
-  IndexASTConsumer(std::shared_ptr<Preprocessor> PP,
-                   std::shared_ptr<IndexingContext> IndexCtx)
-      : PP(std::move(PP)), IndexCtx(std::move(IndexCtx)) {}
-
-protected:
-  void Initialize(ASTContext &Context) override {
-    IndexCtx->setASTContext(Context);
-    IndexCtx->getDataConsumer().initialize(Context);
-    IndexCtx->getDataConsumer().setPreprocessor(PP);
-  }
-
-  bool HandleTopLevelDecl(DeclGroupRef DG) override {
-    return IndexCtx->indexDeclGroupRef(DG);
-  }
-
-  void HandleInterestingDecl(DeclGroupRef DG) override {
-    // Ignore deserialized decls.
-  }
-
-  void HandleTopLevelDeclInObjCContainer(DeclGroupRef DG) override {
-    IndexCtx->indexDeclGroupRef(DG);
-  }
-
-  void HandleTranslationUnit(ASTContext &Ctx) override {
-  }
-};
-
-class IndexPPCallbacks : public PPCallbacks {
+class IndexPPCallbacks final : public PPCallbacks {
   std::shared_ptr<IndexingContext> IndexCtx;
 
 public:
   IndexPPCallbacks(std::shared_ptr<IndexingContext> IndexCtx)
       : IndexCtx(std::move(IndexCtx)) {}
 
   void MacroExpands(const Token &MacroNameTok, const MacroDefinition &MD,
                     SourceRange Range, const MacroArgs *Args) override {
void MacroUndefined(const Token &MacroNameTok, const MacroDefinition &MD,
     if (!MD.getMacroInfo())  // Ignore noop #undef.
       return;
     IndexCtx->handleMacroUndefined(*MacroNameTok.getIdentifierInfo(),
                                    MacroNameTok.getLocation(),
                                    *MD.getMacroInfo());
   }
 };
 
-class IndexActionBase {
-protected:
+class IndexASTConsumer final : public ASTConsumer {
   std::shared_ptr<IndexDataConsumer> DataConsumer;
   std::shared_ptr<IndexingContext> IndexCtx;
+  std::shared_ptr<Preprocessor> PP;
 
-  IndexActionBase(std::shared_ptr<IndexDataConsumer> dataConsumer,
-                  IndexingOptions Opts)
-      : DataConsumer(std::move(dataConsumer)),
-        IndexCtx(new IndexingContext(Opts, *DataConsumer)) {}
+public:
+  IndexASTConsumer(std::shared_ptr<IndexDataConsumer> DataConsumer,
+                   const IndexingOptions &Opts,
+                   std::shared_ptr<Preprocessor> PP)
+      : DataConsumer(std::move(DataConsumer)),
+        IndexCtx(new IndexingContext(Opts, *this->DataConsumer)),
+        PP(std::move(PP)) {
+    assert(this->DataConsumer != nullptr);
+    assert(this->PP != nullptr);
+  }
 
-  std::unique_ptr<IndexASTConsumer>
-  createIndexASTConsumer(CompilerInstance &CI) {
-    return std::make_unique<IndexASTConsumer>(CI.getPreprocessorPtr(),
-                                               IndexCtx);
+protected:
+  void Initialize(ASTContext &Context) override {
+    IndexCtx->setASTContext(Context);
+    IndexCtx->getDataConsumer().initialize(Context);
+    IndexCtx->getDataConsumer().setPreprocessor(PP);
+    PP->addPPCallbacks(std::make_unique<IndexPPCallbacks>(IndexCtx));
+  }
+
+  bool HandleTopLevelDecl(DeclGroupRef DG) override {
+    return IndexCtx->indexDeclGroupRef(DG);
   }
 
-  std::unique_ptr<PPCallbacks> createIndexPPCallbacks() {
-    return std::make_unique<IndexPPCallbacks>(IndexCtx);
+  void HandleInterestingDecl(DeclGroupRef DG) override {
+    // Ignore deserialized decls.
   }
 
-  void finish() {
+  void HandleTopLevelDeclInObjCContainer(DeclGroupRef DG) override {
+    IndexCtx->indexDeclGroupRef(DG);
+  }
+
+  void HandleTranslationUnit(ASTContext &Ctx) override {
     DataConsumer->finish();
   }
 };
 
-class IndexAction : public ASTFrontendAction, IndexActionBase {
+class IndexActionBase {
+protected:
+  std::shared_ptr<IndexDataConsumer> DataConsumer;
+  IndexingOptions Opts;
+
+  IndexActionBase(std::shared_ptr<IndexDataConsumer> DataConsumer,
+                  IndexingOptions Opts)
+      : DataConsumer(std::move(DataConsumer)), Opts(Opts) {
+    assert(this->DataConsumer != nullptr);
+  }
+
+  std::unique_ptr<IndexASTConsumer>
+  createIndexASTConsumer(CompilerInstance &CI) {
+    return std::make_unique<IndexASTConsumer>(DataConsumer, Opts,
+                                              CI.getPreprocessorPtr());
+  }
+};
+
+class IndexAction final : public ASTFrontendAction, IndexActionBase {
 public:
   IndexAction(std::shared_ptr<IndexDataConsumer> DataConsumer,
               IndexingOptions Opts)
     : IndexActionBase(std::move(DataConsumer), Opts) {}
 
 protected:
   std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
                                                  StringRef InFile) override {
     return createIndexASTConsumer(CI);
   }
-
-  bool BeginSourceFileAction(clang::CompilerInstance &CI) override {
-    CI.getPreprocessor().addPPCallbacks(createIndexPPCallbacks());
-    return true;
-  }
-
-  void EndSourceFileAction() override {
-    FrontendAction::EndSourceFileAction();
-    finish();
-  }
 };
 
-class WrappingIndexAction : public WrapperFrontendAction, IndexActionBase {
-  bool IndexActionFailed = false;
-
+class WrappingIndexAction final : public WrapperFrontendAction,
+                                  IndexActionBase {
 public:
   WrappingIndexAction(std::unique_ptr<FrontendAction> WrappedAction,
                       std::shared_ptr<IndexDataConsumer> DataConsumer,
                       IndexingOptions Opts)
     : WrapperFrontendAction(std::move(WrappedAction)),
       IndexActionBase(std::move(DataConsumer), Opts) {}
 
 protected:
   std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
                                                  StringRef InFile) override {
     auto OtherConsumer = WrapperFrontendAction::CreateASTConsumer(CI, InFile);
     if (!OtherConsumer) {
-      IndexActionFailed = true;
       return nullptr;
     }
 
     std::vector<std::unique_ptr<ASTConsumer>> Consumers;
     Consumers.push_back(std::move(OtherConsumer));
     Consumers.push_back(createIndexASTConsumer(CI));
     return std::make_unique<MultiplexConsumer>(std::move(Consumers));
   }
-
-  bool BeginSourceFileAction(clang::CompilerInstance &CI) override {
-    WrapperFrontendAction::BeginSourceFileAction(CI);
-    CI.getPreprocessor().addPPCallbacks(createIndexPPCallbacks());
-    return true;
-  }
-
-  void EndSourceFileAction() override {
-    // Invoke wrapped action's method.
-    WrapperFrontendAction::EndSourceFileAction();
-    if (!IndexActionFailed)
-      finish();
-  }
 };
 
 } // anonymous namespace
 
 std::unique_ptr<FrontendAction>
 index::createIndexingAction(std::shared_ptr<IndexDataConsumer> DataConsumer,
                             IndexingOptions Opts,
                             std::unique_ptr<FrontendAction> WrappedAction) {
+  assert(DataConsumer != nullptr);
+
   if (WrappedAction)
     return std::make_unique<WrappingIndexAction>(std::move(WrappedAction),
-                                                  std::move(DataConsumer),
-                                                  Opts);
+                                                 std::move(DataConsumer), Opts);
   return std::make_unique<IndexAction>(std::move(DataConsumer), Opts);
 }
 
 static bool topLevelDeclVisitor(void *context, const Decl *D) {
   IndexingContext &IndexCtx = *static_cast<IndexingContext*>(context);
   return IndexCtx.indexTopLevelDecl(D);
 }