[clangd] NFC: Use buildCompilerInvocation in CodeComplete

[ilya-biryukov]

clangd/CodeComplete.cpp

1028–1029

remove this line, buildCompilerInvocation handles this for us

1029–1030

remove this line, buildCompilerInvocation handles this for us

[ilya-biryukov]

Adding Sam as a reviewer, IIRC we used to have buildCompilerInvocation here and he was the one who inlined it.
I would personally LGTM this change (with the two comments fixed), but Sam might have a different opinion on whether this should be outlined again.

[sammccall]

In D56860#1365432, @ilya-biryukov wrote:

Adding Sam as a reviewer, IIRC we used to have buildCompilerInvocation here and he was the one who inlined it.
I would personally LGTM this change (with the two comments fixed), but Sam might have a different opinion on whether this should be outlined again.

I inlined it because buildCompilerInvocation was private, and I was moving CodeComplete out of ClangdUnit.cpp in rL319655.
The function was made public in rL333737 as part of a larger change.
I don't really object to the patch as it is, but if you're open to suggestions...

I'd prefer to have fewer dependencies between CodeComplete and ClangdUnit, as the latter has historically been a hairball.
Can we move buildCompilerInvocation and ParseInputs into Compiler.h, which is simplified APIs for invoking the compiler?
Both ClangdUnit and CodeComplete can certainly use Compiler, no question.

[kadircet]

• Moved ParseInputs and buildCompilerInvocation into Compiler.h

[sammccall]

Thank you!

clangd/ClangdUnit.h

 //===--- ClangdUnit.h --------------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDUNIT_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGDUNIT_H
 
-#include "../clang-tidy/ClangTidyOptions.h"
+#include "Compiler.h"
 #include "Diagnostics.h"
 #include "FS.h"
 #include "Function.h"
 #include "Headers.h"
 #include "Path.h"
 #include "Protocol.h"
 #include "clang/Frontend/FrontendAction.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
struct PreambleData {
   // Processes like code completions and go-to-definitions will need #include
   // information, and their compile action skips preamble range.
   IncludeStructure Includes;
   // Cache of FS operations performed when building the preamble.
   // When reusing a preamble, this cache can be consumed to save IO.
   std::unique_ptr<PreambleFileStatusCache> StatCache;
 };
 
-/// Information required to run clang, e.g. to parse AST or do code completion.
-struct ParseInputs {
-  tooling::CompileCommand CompileCommand;
-  IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS;
-  std::string Contents;
-  tidy::ClangTidyOptions ClangTidyOpts;
-};
-
 /// Stores and provides access to parsed AST.
 class ParsedAST {
 public:
   /// Attempts to run Clang and store parsed AST. If \p Preamble is non-null
   /// it is reused during parsing.
   static llvm::Optional<ParsedAST>
   build(std::unique_ptr<clang::CompilerInvocation> CI,
         std::shared_ptr<const PreambleData> Preamble,
private:
   // top-level decls from the preamble.
   std::vector<Decl *> LocalTopLevelDecls;
   IncludeStructure Includes;
 };
 
 using PreambleParsedCallback =
     std::function<void(ASTContext &, std::shared_ptr<clang::Preprocessor>)>;
 
-/// Builds compiler invocation that could be used to build AST or preamble.
-std::unique_ptr<CompilerInvocation>
-buildCompilerInvocation(const ParseInputs &Inputs);
-
 /// Rebuild the preamble for the new inputs unless the old one can be reused.
 /// If \p OldPreamble can be reused, it is returned unchanged.
 /// If \p OldPreamble is null, always builds the preamble.
 /// If \p PreambleCallback is set, it will be run on top of the AST while
 /// building the preamble. Note that if the old preamble was reused, no AST is
 /// built and, therefore, the callback will not be executed.
 std::shared_ptr<const PreambleData>
 buildPreamble(PathRef FileName, CompilerInvocation &CI,

clangd/ClangdUnit.cpp

ParsedAST::ParsedAST(std::shared_ptr<const PreambleData> Preamble,
     : Preamble(std::move(Preamble)), Clang(std::move(Clang)),
       Action(std::move(Action)), Diags(std::move(Diags)),
       LocalTopLevelDecls(std::move(LocalTopLevelDecls)),
       Includes(std::move(Includes)) {
   assert(this->Clang);
   assert(this->Action);
 }
 
-std::unique_ptr<CompilerInvocation>
-buildCompilerInvocation(const ParseInputs &Inputs) {
-  std::vector<const char *> ArgStrs;
-  for (const auto &S : Inputs.CompileCommand.CommandLine)
-    ArgStrs.push_back(S.c_str());
-
-  if (Inputs.FS->setCurrentWorkingDirectory(Inputs.CompileCommand.Directory)) {
-    log("Couldn't set working directory when creating compiler invocation.");
-    // We proceed anyway, our lit-tests rely on results for non-existing working
-    // dirs.
-  }
-
-  // FIXME(ibiryukov): store diagnostics from CommandLine when we start
-  // reporting them.
-  IgnoreDiagnostics IgnoreDiagnostics;
-  llvm::IntrusiveRefCntPtr<DiagnosticsEngine> CommandLineDiagsEngine =
-      CompilerInstance::createDiagnostics(new DiagnosticOptions,
-                                          &IgnoreDiagnostics, false);
-  std::unique_ptr<CompilerInvocation> CI = createInvocationFromCommandLine(
-      ArgStrs, CommandLineDiagsEngine, Inputs.FS);
-  if (!CI)
-    return nullptr;
-  // createInvocationFromCommandLine sets DisableFree.
-  CI->getFrontendOpts().DisableFree = false;
-  CI->getLangOpts()->CommentOpts.ParseAllComments = true;
-  return CI;
-}
-
 std::shared_ptr<const PreambleData>
 buildPreamble(PathRef FileName, CompilerInvocation &CI,
               std::shared_ptr<const PreambleData> OldPreamble,
               const tooling::CompileCommand &OldCompileCommand,
               const ParseInputs &Inputs,
               std::shared_ptr<PCHContainerOperations> PCHs, bool StoreInMemory,
               PreambleParsedCallback PreambleCallback) {
   // Note that we don't need to copy the input contents, preamble can live

clangd/CodeComplete.cpp

 
 // Invokes Sema code completion on a file.
 // If \p Includes is set, it will be updated based on the compiler invocation.
 bool semaCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer,
                       const clang::CodeCompleteOptions &Options,
                       const SemaCompleteInput &Input,
                       IncludeStructure *Includes = nullptr) {
   trace::Span Tracer("Sema completion");
-  std::vector<const char *> ArgStrs;
-  for (const auto &S : Input.Command.CommandLine)
-    ArgStrs.push_back(S.c_str());
-
-  if (Input.VFS->setCurrentWorkingDirectory(Input.Command.Directory)) {
-    log("Couldn't set working directory");
-    // We run parsing anyway, our lit-tests rely on results for non-existing
-    // working dirs.
-  }
-
   llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS = Input.VFS;
   if (Input.Preamble && Input.Preamble->StatCache)
     VFS = Input.Preamble->StatCache->getConsumingFS(std::move(VFS));
-  IgnoreDiagnostics DummyDiagsConsumer;
-  auto CI = createInvocationFromCommandLine(
-      ArgStrs,
-      CompilerInstance::createDiagnostics(new DiagnosticOptions,
-                                          &DummyDiagsConsumer, false),
-      VFS);
+  auto CI =
+      buildCompilerInvocation(ParseInputs{Input.Command, VFS, Input.Contents});
   if (!CI) {
     elog("Couldn't create CompilerInvocation");
     return false;
   }
   auto &FrontendOpts = CI->getFrontendOpts();
-  FrontendOpts.DisableFree = false;
   FrontendOpts.SkipFunctionBodies = true;
-  CI->getLangOpts()->CommentOpts.ParseAllComments = true;
   // Disable typo correction in Sema.
   CI->getLangOpts()->SpellChecking = false;
   // Setup code completion.
   FrontendOpts.CodeCompleteOpts = Options;
   FrontendOpts.CodeCompletionAt.FileName = Input.FileName;
   auto Offset = positionToOffset(Input.Contents, Input.Pos);
   if (!Offset) {
     elog("Code completion position was invalid {0}", Offset.takeError());
     return false;
bool semaCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer,
   // However, if we're completing *inside* the preamble section of the draft,
   // overriding the preamble will break sema completion. Fortunately we can just
   // skip all includes in this case; these completions are really simple.
   bool CompletingInPreamble =
       ComputePreambleBounds(*CI->getLangOpts(), ContentsBuffer.get(), 0).Size >
       *Offset;
   // NOTE: we must call BeginSourceFile after prepareCompilerInstance. Otherwise
   // the remapped buffers do not get freed.
+  IgnoreDiagnostics DummyDiagsConsumer;
   auto Clang = prepareCompilerInstance(
       std::move(CI),
       (Input.Preamble && !CompletingInPreamble) ? &Input.Preamble->Preamble
                                                 : nullptr,
       std::move(ContentsBuffer), std::move(Input.PCHs), std::move(VFS),
       DummyDiagsConsumer);
   Clang->getPreprocessorOpts().SingleFileParseMode = CompletingInPreamble;
   Clang->setCodeCompletionConsumer(Consumer.release());

clangd/Compiler.h

 // ClangdUnit takes care of much of this, but some features like CodeComplete
 // run their own compile actions that share logic.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_COMPILER_H
 #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_COMPILER_H
 
+#include "../clang-tidy/ClangTidyOptions.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/CompilerInvocation.h"
 #include "clang/Frontend/PrecompiledPreamble.h"
+#include "clang/Tooling/CompilationDatabase.h"
 
 namespace clang {
 namespace clangd {
 
 class IgnoreDiagnostics : public DiagnosticConsumer {
 public:
   static void log(DiagnosticsEngine::Level DiagLevel,
                   const clang::Diagnostic &Info);
 
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const clang::Diagnostic &Info) override;
 };
 
+/// Information required to run clang, e.g. to parse AST or do code completion.
+struct ParseInputs {
+  tooling::CompileCommand CompileCommand;
+  IntrusiveRefCntPtr<llvm::vfs::FileSystem> FS;
+  std::string Contents;
+  tidy::ClangTidyOptions ClangTidyOpts;
+};
+
+/// Builds compiler invocation that could be used to build AST or preamble.
+std::unique_ptr<CompilerInvocation>
+buildCompilerInvocation(const ParseInputs &Inputs);
+
 /// Creates a compiler instance, configured so that:
 ///   - Contents of the parsed file are remapped to \p MainFile.
 ///   - Preamble is overriden to use PCH passed to this function. It means the
 ///     changes to the preamble headers or files included in the preamble are
 ///     not visible to this compiler instance.
 ///   - llvm::vfs::FileSystem is used for all underlying file accesses. The
 ///     actual vfs used by the compiler may be an overlay over the passed vfs.
 /// Returns null on errors. When non-null value is returned, it is expected to

clangd/Compiler.cpp

void IgnoreDiagnostics::log(DiagnosticsEngine::Level DiagLevel,
   clangd::vlog("Ignored diagnostic. {0}{1}", Location, Message);
 }
 
 void IgnoreDiagnostics::HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                                          const clang::Diagnostic &Info) {
   IgnoreDiagnostics::log(DiagLevel, Info);
 }
 
+std::unique_ptr<CompilerInvocation>
+buildCompilerInvocation(const ParseInputs &Inputs) {
+  std::vector<const char *> ArgStrs;
+  for (const auto &S : Inputs.CompileCommand.CommandLine)
+    ArgStrs.push_back(S.c_str());
+
+  if (Inputs.FS->setCurrentWorkingDirectory(Inputs.CompileCommand.Directory)) {
+    log("Couldn't set working directory when creating compiler invocation.");
+    // We proceed anyway, our lit-tests rely on results for non-existing working
+    // dirs.
+  }
+
+  // FIXME(ibiryukov): store diagnostics from CommandLine when we start
+  // reporting them.
+  IgnoreDiagnostics IgnoreDiagnostics;
+  llvm::IntrusiveRefCntPtr<DiagnosticsEngine> CommandLineDiagsEngine =
+      CompilerInstance::createDiagnostics(new DiagnosticOptions,
+                                          &IgnoreDiagnostics, false);
+  std::unique_ptr<CompilerInvocation> CI = createInvocationFromCommandLine(
+      ArgStrs, CommandLineDiagsEngine, Inputs.FS);
+  if (!CI)
+    return nullptr;
+  // createInvocationFromCommandLine sets DisableFree.
+  CI->getFrontendOpts().DisableFree = false;
+  CI->getLangOpts()->CommentOpts.ParseAllComments = true;
+  return CI;
+}
+
 std::unique_ptr<CompilerInstance>
 prepareCompilerInstance(std::unique_ptr<clang::CompilerInvocation> CI,
                         const PrecompiledPreamble *Preamble,
                         std::unique_ptr<llvm::MemoryBuffer> Buffer,
                         std::shared_ptr<PCHContainerOperations> PCHs,
                         llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> VFS,
                         DiagnosticConsumer &DiagsClient) {
   assert(VFS && "VFS is null");