[clang] Add PPCallbacks list to preprocessor when building a preacompiled preamble.

[Nebiroth]

Summary

Revision D38639 needs this commit in order to properly make open definition calls on include statements work.
Since this modifies clang and not clangd, I decided to include it in a different patch.

[malaperle]

Can you revert the formatting changes in places you haven’t touched (mainly astunit.cpp)? I think you should be able to do that with git checkout -p HEAD~1

[ilya-biryukov]

Could you please take a different approach and add the callbacks you need into PreambleCallbacks class?
It already has this one:

virtual void HandleMacroDefined(const Token &MacroNameTok,
                               const MacroDirective *MD);

Otherwise we'll be creating too many ways to do the same thing.

We should also definitely get rid of all the unrelated formatting changes.

[Nebiroth]

Reverted formatting changes to ASTUnit

[ilya-biryukov]

lib/Frontend/PrecompiledPreamble.cpp

242 ↗

(On Diff #125815)

Could we add a method to PreambleCallbacks to create PPCallbacks instead?
Otherwise we have both MacroDefined in PreambleCallbacks and a separate set of PPCallbacks, so we have two ways of doing the same thing.

class PreambleCallbacks {
public:
// ...

  /// Remove this.
  virtual void HandleMacroDefined(...);

  // Add this instead.
  virtual std::unique_ptr<PPCallbacks> createPPCallbacks();

}

Alternatively, follow the current pattern in PreambleCallbacks and add some extra functions from the PPCallbacks interface to it. This would not require changes to the existing usages of PrecompiledPreamble in ASTUnit. Albeit, I'd prefer the first option.

class PreambleCallbacks {
public:
// ...

  // Keep this
  virtual void HandleMacroDefined(...);
  // Add the ones you need, e.g.
  virtual void InclusionDirective(...);
  virtual void FileChanged(...);
};

[Nebiroth]

lib/Frontend/PrecompiledPreamble.cpp

242 ↗

(On Diff #125815)

If we were to do that, one would then be required to define a wrapper class for PPCallbacks and create an instance of it inside createPPCallbacks() for the purpose of creating a unique_ptr? Then that unique_ptr would be sent from within the PreambleCallbacks parameter in the Build function?

[ilya-biryukov]

lib/Frontend/PrecompiledPreamble.cpp

242 ↗

(On Diff #125815)

We're already passing our own wrapper around PreambleCallbacks anyway (see PreambleMacroCallbacks), we'll pass the unique_ptr<PPCallbacks> instead.
But, yes, the clients will have to write their own implementation of PPCallbacks and pass it as unique_ptr. Is there something wrong with that?

Or, have I misunderstood the question entirely?

[Nebiroth]

lib/Frontend/PrecompiledPreamble.cpp

242 ↗

(On Diff #125815)

No this is fine. I was just making sure I understood correctly.

[Nebiroth]

Removed unique_ptr parameter in PrecompiledPreamble::Build
Added method createPPCallbacks() in PreambleCallback to be overriden

[malaperle]

include/clang/Frontend/PrecompiledPreamble.h

263 ↗

(On Diff #126430)

It doesn't add, it creates.It's also not a "list". Is it true that it will process includes outside the preamble? I would think building the preamble stopped at the end of the preamble.

lib/Frontend/PrecompiledPreamble.cpp

354 ↗

(On Diff #126430)

extract this to a local variable

[Nebiroth]

Minor code cleanup

[malaperle]

Looks good to me. But this is not really my area :)

[ilya-biryukov]

LGTM modulo a few comments.
AFAIK, @Nebiroth does not have a commit access, so I'll land this with minor tweaks to unblock the other patch.

include/clang/Frontend/PrecompiledPreamble.h

261 ↗

(On Diff #126984)

This method can now be removed as its use-case is now covered by createPPCallbacks

lib/Frontend/PrecompiledPreamble.cpp

716 ↗

(On Diff #126984)

Please clang-format the code when submitting patches.

cfe/trunk/include/clang/Frontend/PrecompiledPreamble.h

public:
   virtual void AfterExecute(CompilerInstance &CI);
   /// Called after PCH has been emitted. \p Writer may be used to retrieve
   /// information about AST, serialized in PCH.
   virtual void AfterPCHEmitted(ASTWriter &Writer);
   /// Called for each TopLevelDecl.
   /// NOTE: To allow more flexibility a custom ASTConsumer could probably be
   /// used instead, but having only this method allows a simpler API.
   virtual void HandleTopLevelDecl(DeclGroupRef DG);
-  /// Called for each macro defined in the Preamble.
-  /// NOTE: To allow more flexibility a custom PPCallbacks could probably be
-  /// used instead, but having only this method allows a simpler API.
-  virtual void HandleMacroDefined(const Token &MacroNameTok,
-                                  const MacroDirective *MD);
+  /// Creates wrapper class for PPCallbacks so we can also process information
+  /// about includes that are inside of a preamble
+  virtual std::unique_ptr<PPCallbacks> createPPCallbacks();
 };
 
 enum class BuildPreambleError {
   PreambleIsEmpty = 1,
   CouldntCreateTempFile,
   CouldntCreateTargetInfo,
   CouldntCreateVFSOverlay,
   BeginSourceFileFailed,

cfe/trunk/lib/Frontend/ASTUnit.cpp

for (Decl *D : DG) {
       // fundamental problem in the parser right now.
       if (isa<ObjCMethodDecl>(D))
         continue;
       AddTopLevelDeclarationToHash(D, Hash);
       TopLevelDecls.push_back(D);
     }
   }
 
-  void HandleMacroDefined(const Token &MacroNameTok,
-                          const MacroDirective *MD) override {
-    AddDefinedMacroToHash(MacroNameTok, Hash);
+  std::unique_ptr<PPCallbacks> createPPCallbacks() override {
+    return llvm::make_unique<MacroDefinitionTrackerPPCallbacks>(Hash);
   }
 
 private:
   unsigned Hash = 0;
   std::vector<Decl *> TopLevelDecls;
   std::vector<serialization::DeclID> TopLevelDeclIDs;
   llvm::SmallVector<ASTUnit::StandaloneDiagnostic, 4> PreambleDiags;
 };

cfe/trunk/lib/Frontend/PrecompiledPreamble.cpp

 void TemporaryFiles::removeFile(StringRef File) {
   llvm::MutexGuard Guard(Mutex);
   auto WasPresent = Files.erase(File);
   (void)WasPresent;
   assert(WasPresent && "File was not tracked");
   llvm::sys::fs::remove(File);
 }
 
-class PreambleMacroCallbacks : public PPCallbacks {
-public:
-  PreambleMacroCallbacks(PreambleCallbacks &Callbacks) : Callbacks(Callbacks) {}
-
-  void MacroDefined(const Token &MacroNameTok,
-                    const MacroDirective *MD) override {
-    Callbacks.HandleMacroDefined(MacroNameTok, MD);
-  }
-
-private:
-  PreambleCallbacks &Callbacks;
-};
-
 class PrecompilePreambleAction : public ASTFrontendAction {
 public:
   PrecompilePreambleAction(std::string *InMemStorage,
                            PreambleCallbacks &Callbacks)
       : InMemStorage(InMemStorage), Callbacks(Callbacks) {}
 
   std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
                                                  StringRef InFile) override;
if (InMemStorage) {
     OS = GeneratePCHAction::CreateOutputFile(CI, InFile, OutputFile);
   }
   if (!OS)
     return nullptr;
 
   if (!CI.getFrontendOpts().RelocatablePCH)
     Sysroot.clear();
 
-  CI.getPreprocessor().addPPCallbacks(
-      llvm::make_unique<PreambleMacroCallbacks>(Callbacks));
   return llvm::make_unique<PrecompilePreambleConsumer>(
       *this, CI.getPreprocessor(), Sysroot, std::move(OS));
 }
 
 template <class T> bool moveOnNoError(llvm::ErrorOr<T> Val, T &Output) {
   if (!Val)
     return false;
   Output = std::move(*Val);
llvm::ErrorOr<PrecompiledPreamble> PrecompiledPreamble::Build(
   }
 
   std::unique_ptr<PrecompilePreambleAction> Act;
   Act.reset(new PrecompilePreambleAction(
       StoreInMemory ? &Storage.asMemory().Data : nullptr, Callbacks));
   if (!Act->BeginSourceFile(*Clang.get(), Clang->getFrontendOpts().Inputs[0]))
     return BuildPreambleError::BeginSourceFileFailed;
 
+  std::unique_ptr<PPCallbacks> DelegatedPPCallbacks =
+      Callbacks.createPPCallbacks();
+  if (DelegatedPPCallbacks)
+    Clang->getPreprocessor().addPPCallbacks(std::move(DelegatedPPCallbacks));
+
   Act->Execute();
 
   // Run the callbacks.
   Callbacks.AfterExecute(*Clang);
 
   Act->EndSourceFile();
 
   if (!Act->hasEmittedPreamblePCH())
if (Storage.getKind() == PCHStorage::Kind::TempFile) {
     auto Buf = llvm::MemoryBuffer::getMemBuffer(Storage.asMemory().Data);
     VFS = createVFSOverlayForPreamblePCH(PCHPath, std::move(Buf), VFS);
   }
 }
 
 void PreambleCallbacks::AfterExecute(CompilerInstance &CI) {}
 void PreambleCallbacks::AfterPCHEmitted(ASTWriter &Writer) {}
 void PreambleCallbacks::HandleTopLevelDecl(DeclGroupRef DG) {}
-void PreambleCallbacks::HandleMacroDefined(const Token &MacroNameTok,
-                                           const MacroDirective *MD) {}
+std::unique_ptr<PPCallbacks> PreambleCallbacks::createPPCallbacks() {
+  return nullptr;
+}
 
 std::error_code clang::make_error_code(BuildPreambleError Error) {
   return std::error_code(static_cast<int>(Error), BuildPreambleErrorCategory());
 }
 
 const char *BuildPreambleErrorCategory::name() const noexcept {
   return "build-preamble.error";
 }