[clangd] Do not write comments into Preamble PCH

[ilya-biryukov]

Summary

To avoid wasting time deserializing them on code completion and
further reparses.

We do not use the comments anyway, because we cannot rely on the file
contents staying the same for reparses that reuse the prebuilt
preamble PCH.

[sammccall]

Very nice!

clangd/ClangdUnit.cpp

327

nit: these are comment *locations* right?
(Relevant because if we were writing the actual comments, we may actually want this on!)

[ilya-biryukov]

• Fix a comment

clangd/ClangdUnit.cpp

 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/FrontendActions.h"
 #include "clang/Frontend/Utils.h"
 #include "clang/Index/IndexDataConsumer.h"
 #include "clang/Index/IndexingAction.h"
 #include "clang/Lex/Lexer.h"
 #include "clang/Lex/MacroInfo.h"
 #include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/PreprocessorOptions.h"
 #include "clang/Sema/Sema.h"
 #include "clang/Serialization/ASTWriter.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/CrashRecoveryContext.h"
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
std::shared_ptr<const PreambleData> clangd::buildPreamble(
   IntrusiveRefCntPtr<DiagnosticsEngine> PreambleDiagsEngine =
       CompilerInstance::createDiagnostics(&CI.getDiagnosticOpts(),
                                           &PreambleDiagnostics, false);
 
   // Skip function bodies when building the preamble to speed up building
   // the preamble and make it smaller.
   assert(!CI.getFrontendOpts().SkipFunctionBodies);
   CI.getFrontendOpts().SkipFunctionBodies = true;
+  // We don't want to write comments into PCH. They are racy and slow to read
+  // back. We rely on dynamic index for the comments instead.
+  CI.getPreprocessorOpts().WriteCommentListToPCH = false;
 
   CppFilePreambleCallbacks SerializedDeclsCollector(FileName, PreambleCallback);
   if (Inputs.FS->setCurrentWorkingDirectory(Inputs.CompileCommand.Directory)) {
     log("Couldn't set working directory when building the preamble.");
     // We proceed anyway, our lit-tests rely on results for non-existing working
     // dirs.
   }
   auto BuiltPreamble = PrecompiledPreamble::Build(

clangd/CodeCompletionStrings.cpp

 void appendEscapeSnippet(const llvm::StringRef Text, std::string *Out) {
   for (const auto Character : Text) {
     if (Character == '$' || Character == '}' || Character == '\\')
       Out->push_back('\\');
     Out->push_back(Character);
   }
 }
 
-bool canRequestComment(const ASTContext &Ctx, const NamedDecl &D,
-                       bool CommentsFromHeaders) {
-  if (CommentsFromHeaders)
-    return true;
-  auto &SourceMgr = Ctx.getSourceManager();
-  // Accessing comments for decls from  invalid preamble can lead to crashes.
-  // So we only return comments from the main file when doing code completion.
-  // For indexing, we still read all the comments.
-  // FIXME: find a better fix, e.g. store file contents in the preamble or get
-  // doc comments from the index.
-  auto canRequestForDecl = [&](const NamedDecl &D) -> bool {
-    for (auto *Redecl : D.redecls()) {
-      auto Loc = SourceMgr.getSpellingLoc(Redecl->getLocation());
-      if (!SourceMgr.isWrittenInMainFile(Loc))
-        return false;
-    }
-    return true;
-  };
-  // First, check the decl itself.
-  if (!canRequestForDecl(D))
-    return false;
-  // Completion also returns comments for properties, corresponding to ObjC
-  // methods.
-  const ObjCMethodDecl *M = dyn_cast<ObjCMethodDecl>(&D);
-  const ObjCPropertyDecl *PDecl = M ? M->findPropertyDecl() : nullptr;
-  return !PDecl || canRequestForDecl(*PDecl);
-}
-
 bool LooksLikeDocComment(llvm::StringRef CommentText) {
   // We don't report comments that only contain "special" chars.
   // This avoids reporting various delimiters, like:
   //   =================
   //   -----------------
   //   *****************
   return CommentText.find_first_not_of("/*-= \t\r\n") != llvm::StringRef::npos;
 }
std::string getDocComment(const ASTContext &Ctx,
   auto *Decl = Result.getDeclaration();
   if (!Decl || llvm::isa<NamespaceDecl>(Decl)) {
     // Namespaces often have too many redecls for any particular redecl comment
     // to be useful. Moreover, we often confuse file headers or generated
     // comments with namespace comments. Therefore we choose to just ignore
     // the comments for namespaces.
     return "";
   }
-  if (!canRequestComment(Ctx, *Decl, CommentsFromHeaders))
-    return "";
-
   const RawComment *RC = getCompletionComment(Ctx, Decl);
   if (!RC)
     return "";
+
+  // Sanity check that the comment does not come from the PCH. We choose to not
+  // write them into PCH, because they are racy and slow to load.
+  assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getLocStart()));
   std::string Doc = RC->getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics());
   if (!LooksLikeDocComment(Doc))
     return "";
   return Doc;
 }
 
 std::string
 getParameterDocComment(const ASTContext &Ctx,
                        const CodeCompleteConsumer::OverloadCandidate &Result,
                        unsigned ArgIndex, bool CommentsFromHeaders) {
   auto *Func = Result.getFunction();
-  if (!Func || !canRequestComment(Ctx, *Func, CommentsFromHeaders))
+  if (!Func)
     return "";
   const RawComment *RC = getParameterComment(Ctx, Result, ArgIndex);
   if (!RC)
     return "";
+  // Sanity check that the comment does not come from the PCH. We choose to not
+  // write them into PCH, because they are racy and slow to load.
+  assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getLocStart()));
   std::string Doc = RC->getFormattedText(Ctx.getSourceManager(), Ctx.getDiagnostics());
   if (!LooksLikeDocComment(Doc))
     return "";
   return Doc;
 }
 
 void getSignature(const CodeCompletionString &CCS, std::string *Signature,
                   std::string *Snippet, std::string *RequiredQualifiers) {