[clangd] GlobalCompilationDatabase interface changes

[sammccall]

Summary

• GlobalCompilationDatabase now returns a single command (that's all we use)
• fallback flags are now part of the GlobalCompilationDatabase. There's a default implementation that they can optionally customize.
• this allows us to avoid invoking the fallback logic on two separate codepaths
• race on extra flags fixed by locking the mutex
• made GCD const-correct (DBGCD does have mutating methods)

[hokein]

LGTM.

clangd/ClangdUnitStore.cpp

67 ↗

(On Diff #125151)

nit: you can use ResourceDir.str().

clang-tools-extra/trunk/clangd/ClangdUnitStore.cpp

CppFileCollection::recreateFileIfCompileCommandChanged(
   }
   Result.FileInCollection = It->second;
   return Result;
 }
 
 tooling::CompileCommand
 CppFileCollection::getCompileCommand(GlobalCompilationDatabase &CDB,
                                      PathRef File, PathRef ResourceDir) {
-  std::vector<tooling::CompileCommand> Commands = CDB.getCompileCommands(File);
-  if (Commands.empty())
-    // Add a fake command line if we know nothing.
-    Commands.push_back(getDefaultCompileCommand(File));
+  llvm::Optional<tooling::CompileCommand> C = CDB.getCompileCommand(File);
+  if (!C) // FIXME: Suppress diagnostics? Let the user know?
+    C = CDB.getFallbackCommand(File);
 
   // Inject the resource dir.
   // FIXME: Don't overwrite it if it's already there.
-  Commands.front().CommandLine.push_back("-resource-dir=" +
-                                         std::string(ResourceDir));
-  return std::move(Commands.front());
+  C->CommandLine.push_back("-resource-dir=" + ResourceDir.str());
+  return std::move(*C);
 }
 
 bool CppFileCollection::compileCommandsAreEqual(
     tooling::CompileCommand const &LHS, tooling::CompileCommand const &RHS) {
   // tooling::CompileCommand.Output is ignored, it's not relevant for clangd.
   return LHS.Directory == RHS.Directory &&
          LHS.CommandLine.size() == RHS.CommandLine.size() &&
          std::equal(LHS.CommandLine.begin(), LHS.CommandLine.end(),
                     RHS.CommandLine.begin());
 }

clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h

 class CompilationDatabase;
 struct CompileCommand;
 } // namespace tooling
 
 namespace clangd {
 
 class Logger;
 
-/// Returns a default compile command to use for \p File.
-tooling::CompileCommand getDefaultCompileCommand(PathRef File);
-
 /// Provides compilation arguments used for parsing C and C++ files.
 class GlobalCompilationDatabase {
 public:
   virtual ~GlobalCompilationDatabase() = default;
 
-  virtual std::vector<tooling::CompileCommand>
-  getCompileCommands(PathRef File) = 0;
+  /// If there are any known-good commands for building this file, returns one.
+  virtual llvm::Optional<tooling::CompileCommand>
+  getCompileCommand(PathRef File) const = 0;
+
+  /// Makes a guess at how to build a file.
+  /// The default implementation just runs clang on the file.
+  /// Clangd should treat the results as unreliable.
+  virtual tooling::CompileCommand getFallbackCommand(PathRef File) const;
 
   /// FIXME(ibiryukov): add facilities to track changes to compilation flags of
   /// existing targets.
 };
 
 /// Gets compile args from tooling::CompilationDatabases built for parent
 /// directories.
 class DirectoryBasedGlobalCompilationDatabase
     : public GlobalCompilationDatabase {
 public:
   DirectoryBasedGlobalCompilationDatabase(
       clangd::Logger &Logger, llvm::Optional<Path> CompileCommandsDir);
 
-  std::vector<tooling::CompileCommand>
-  getCompileCommands(PathRef File) override;
+  /// Scans File's parents looking for compilation databases.
+  /// Any extra flags will be added.
+  llvm::Optional<tooling::CompileCommand>
+  getCompileCommand(PathRef File) const override;
+
+  /// Uses the default fallback command, adding any extra flags.
+  tooling::CompileCommand getFallbackCommand(PathRef File) const override;
 
+  /// Sets the extra flags that should be added to a file.
   void setExtraFlagsForFile(PathRef File, std::vector<std::string> ExtraFlags);
 
 private:
-  tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
-  tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File);
+  tooling::CompilationDatabase *getCompilationDatabase(PathRef File) const;
+  tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File) const;
+  void addExtraFlags(PathRef File, tooling::CompileCommand &C) const;
 
-  std::mutex Mutex;
+  mutable std::mutex Mutex;
   /// Caches compilation databases loaded from directories(keys are
   /// directories).
-  llvm::StringMap<std::unique_ptr<clang::tooling::CompilationDatabase>>
+  mutable llvm::StringMap<std::unique_ptr<clang::tooling::CompilationDatabase>>
       CompilationDatabases;
 
   /// Stores extra flags per file.
   llvm::StringMap<std::vector<std::string>> ExtraFlagsForFile;
   /// Used for logging.
   clangd::Logger &Logger;
   /// Used for command argument pointing to folder where compile_commands.json
   /// is located.
   llvm::Optional<Path> CompileCommandsDir;
 };
 } // namespace clangd
 } // namespace clang
 
 #endif

clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp

 #include "Logger.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 
 namespace clang {
 namespace clangd {
 
-static void addExtraFlags(tooling::CompileCommand &Command,
-                          const std::vector<std::string> &ExtraFlags) {
-  if (ExtraFlags.empty())
-    return;
-  assert(Command.CommandLine.size() >= 2 &&
-         "Expected a command line containing at least 2 arguments, the "
-         "compiler binary and the output file");
-  // The last argument of CommandLine is the name of the input file.
-  // Add ExtraFlags before it.
-  auto It = Command.CommandLine.end();
-  --It;
-  Command.CommandLine.insert(It, ExtraFlags.begin(), ExtraFlags.end());
-}
-
-tooling::CompileCommand getDefaultCompileCommand(PathRef File) {
-  std::vector<std::string> CommandLine{"clang", File.str()};
+tooling::CompileCommand
+GlobalCompilationDatabase::getFallbackCommand(PathRef File) const {
   return tooling::CompileCommand(llvm::sys::path::parent_path(File),
-                                 llvm::sys::path::filename(File), CommandLine,
+                                 llvm::sys::path::filename(File),
+                                 {"clang", File.str()},
                                  /*Output=*/"");
 }
 
 DirectoryBasedGlobalCompilationDatabase::
     DirectoryBasedGlobalCompilationDatabase(
         clangd::Logger &Logger, llvm::Optional<Path> CompileCommandsDir)
     : Logger(Logger), CompileCommandsDir(std::move(CompileCommandsDir)) {}
 
-std::vector<tooling::CompileCommand>
-DirectoryBasedGlobalCompilationDatabase::getCompileCommands(PathRef File) {
-  std::vector<tooling::CompileCommand> Commands;
-
-  auto CDB = getCompilationDatabase(File);
-  if (CDB)
-    Commands = CDB->getCompileCommands(File);
-  if (Commands.empty())
-    Commands.push_back(getDefaultCompileCommand(File));
-
-  auto It = ExtraFlagsForFile.find(File);
-  if (It != ExtraFlagsForFile.end()) {
-    // Append the user-specified flags to the compile commands.
-    for (tooling::CompileCommand &Command : Commands)
-      addExtraFlags(Command, It->second);
-  }
-
-  return Commands;
+llvm::Optional<tooling::CompileCommand>
+DirectoryBasedGlobalCompilationDatabase::getCompileCommand(PathRef File) const {
+  if (auto CDB = getCompilationDatabase(File)) {
+    auto Candidates = CDB->getCompileCommands(File);
+    if (!Candidates.empty()) {
+      addExtraFlags(File, Candidates.front());
+      return std::move(Candidates.front());
+    }
+  }
+  return llvm::None;
+}
+
+tooling::CompileCommand
+DirectoryBasedGlobalCompilationDatabase::getFallbackCommand(
+    PathRef File) const {
+  auto C = GlobalCompilationDatabase::getFallbackCommand(File);
+  addExtraFlags(File, C);
+  return C;
 }
 
 void DirectoryBasedGlobalCompilationDatabase::setExtraFlagsForFile(
     PathRef File, std::vector<std::string> ExtraFlags) {
+  std::lock_guard<std::mutex> Lock(Mutex);
   ExtraFlagsForFile[File] = std::move(ExtraFlags);
 }
 
+void DirectoryBasedGlobalCompilationDatabase::addExtraFlags(
+    PathRef File, tooling::CompileCommand &C) const {
+  std::lock_guard<std::mutex> Lock(Mutex);
+
+  auto It = ExtraFlagsForFile.find(File);
+  if (It == ExtraFlagsForFile.end())
+    return;
+
+  auto &Args = C.CommandLine;
+  assert(Args.size() >= 2 && "Expected at least [compiler, source file]");
+  // The last argument of CommandLine is the name of the input file.
+  // Add ExtraFlags before it.
+  Args.insert(Args.end() - 1, It->second.begin(), It->second.end());
+}
+
 tooling::CompilationDatabase *
-DirectoryBasedGlobalCompilationDatabase::tryLoadDatabaseFromPath(PathRef File) {
+DirectoryBasedGlobalCompilationDatabase::tryLoadDatabaseFromPath(
+    PathRef File) const {
 
   namespace path = llvm::sys::path;
   auto CachedIt = CompilationDatabases.find(File);
 
   assert((path::is_absolute(File, path::Style::posix) ||
           path::is_absolute(File, path::Style::windows)) &&
          "path must be absolute");
 
   if (CachedIt != CompilationDatabases.end())
     return CachedIt->second.get();
   std::string Error = "";
   auto CDB = tooling::CompilationDatabase::loadFromDirectory(File, Error);
   if (CDB) {
     auto Result = CDB.get();
     CompilationDatabases.insert(std::make_pair(File, std::move(CDB)));
     return Result;
   }
 
   return nullptr;
 }
 
 tooling::CompilationDatabase *
-DirectoryBasedGlobalCompilationDatabase::getCompilationDatabase(PathRef File) {
+DirectoryBasedGlobalCompilationDatabase::getCompilationDatabase(
+    PathRef File) const {
   std::lock_guard<std::mutex> Lock(Mutex);
 
   namespace path = llvm::sys::path;
   if (CompileCommandsDir.hasValue()) {
     tooling::CompilationDatabase *ReturnValue =
         tryLoadDatabaseFromPath(CompileCommandsDir.getValue());
     if (ReturnValue == nullptr)
       Logger.log("Failed to find compilation database for " + Twine(File) +

clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp

 public:
   MockCompilationDatabase(bool AddFreestandingFlag) {
     // We have to add -ffreestanding to VFS-specific tests to avoid errors on
     // implicit includes of stdc-predef.h.
     if (AddFreestandingFlag)
       ExtraClangFlags.push_back("-ffreestanding");
   }
 
-  std::vector<tooling::CompileCommand>
-  getCompileCommands(PathRef File) override {
+  llvm::Optional<tooling::CompileCommand>
+  getCompileCommand(PathRef File) const override {
     if (ExtraClangFlags.empty())
-      return {};
-
-    std::vector<std::string> CommandLine;
-    CommandLine.reserve(3 + ExtraClangFlags.size());
-    CommandLine.insert(CommandLine.end(), {"clang", "-fsyntax-only"});
-    CommandLine.insert(CommandLine.end(), ExtraClangFlags.begin(),
-                       ExtraClangFlags.end());
-    CommandLine.push_back(File.str());
+      return llvm::None;
 
+    auto CommandLine = ExtraClangFlags;
+    CommandLine.insert(CommandLine.begin(), "clang");
+    CommandLine.insert(CommandLine.end(), File.str());
     return {tooling::CompileCommand(llvm::sys::path::parent_path(File),
                                     llvm::sys::path::filename(File),
-                                    CommandLine, "")};
+                                    std::move(CommandLine), "")};
   }
 
   std::vector<std::string> ExtraClangFlags;
 };
 
 IntrusiveRefCntPtr<vfs::FileSystem>
 buildTestFS(llvm::StringMap<std::string> const &Files) {
   IntrusiveRefCntPtr<vfs::InMemoryFileSystem> MemFS(