[clangd] Command line arg to specify compile_commands.json path

[Nebiroth]

Summary

Adds compileCommands command line argument to specify an absolute path directly to the requested compile_commands.json for flags.

[ilya-biryukov]

clangd/GlobalCompilationDatabase.cpp

29 ↗

(On Diff #112701)

Please place this flag in ClangdMain.cpp.

29 ↗

(On Diff #112701)

Following naming convention for flags, this should be compile-commands.
Given the semantics of the flag, maybe it's better to name it compile-commands-dir?

30 ↗

(On Diff #112701)

A description is somewhat vague. What is the intention of this flag?

• Always look for compile_commands.json only inside specified directory?
• First look inside specified directory, then proceed as normal (i.e. looking at the parent paths of each source file)?

It looks like the code also looks at the parent directories of compile-commands. Is that the original intention? Maybe consider making it look only inside the directory itself and not its parents?

57 ↗

(On Diff #112701)

Incidental whitespace change?

89 ↗

(On Diff #112701)

Please check command arguments for validity in main().
What is the purpose of find_first_of("/")? Checking for absolute paths? If yes, you could use llvm::sys::path::is_absolute.

97 ↗

(On Diff #112701)

Naming: local variables should be UpperCamelCase

99 ↗

(On Diff #112701)

Maybe extract a function from the for loop body instead of copying it here?
Could be named tryLoadDatabaseFromPath.

113 ↗

(On Diff #112701)

Maybe rewrite to avoid extra nesting?

For example,

if (!badPath) {
    auto result = CDB.get();
    CompilationDatabases.insert(std::make_pair(File, std::move(CDB)));
    return result;
}
// Proceed as before...

138 ↗

(On Diff #112701)

Please consider removing this code duplication by extracting a function from the loop body (also see my previous comment).

[ilya-biryukov]

BTW, if you're only looking for providing extra compilation flags for some files, you could use an extension to LSP that is already in clangd source tree. (See tests from https://reviews.llvm.org/D34947 for more details).
But your change is also useful, as it allows to override the whole compilation database. Would be interested in this change to land regardless of whether the extra flags thing suits you well.

[krasimir]

clangd/GlobalCompilationDatabase.cpp

89 ↗

(On Diff #112701)

Please make CompileCommands a member variable and pass it through the constructor of DirectoryBasedGlobalCompilationDatabase. This is more flexible and, among other things, allows for easier unit testing.

94 ↗

(On Diff #112701)

LLVM style uses no braces for single statement if's.

[Nebiroth]

Added function to avoid code reuse and extra nesting.

[ilya-biryukov]

See my comments.
Could you also clang-format the code please?

clangd/ClangdServer.cpp

150 ↗

(On Diff #113895)

Could you pass llvm::Optional<Path> instead (Path is just a descriptive typedef that we use in place of std::string) and check for empty string in main()?

clangd/GlobalCompilationDatabase.cpp

14 ↗

(On Diff #113895)

There are definitely unneeded includes here. Remove most of them and leave only the required ones?

93 ↗

(On Diff #113895)

Let's simply lookup only inside CompileCommandsDir if it was set.
Otherwise, fallback into looking at parent_paths of the file.

30 ↗

(On Diff #112701)

How about chaning semantics of the flag to only look inside "--compile-commands-dir" and not its parent paths?
I think it makes the behavior much less surprising. The whole point of looking in parent paths is that typically compile_commands.json is put inside the project root.

The flag that you propose seems to have a different purpose of providing clangd with custom compilation flags.

clangd/GlobalCompilationDatabase.h

38 ↗

(On Diff #113895)

GlobalCompilationDatabase is an interface class and is designed specifically to not contain any fields.
Could you please do the following?

• Move this field into DirectoryBasedGlobalCompilationDatabase.
• Make it private and initialize it in constructor.

clangd/tool/ClangdMain.cpp

29 ↗

(On Diff #113895)

Could you use descriptive Path typedef instead of std::string here and in other places?

30 ↗

(On Diff #113895)

Maybe rename this (and other occurences) to CompileCommandsDir?

43 ↗

(On Diff #113895)

• CompileCommands is empty by default, don't show the error message in that case.
• If value of CompileCommands is not absolute or the path does not exist, set CompileCommands to empty string after reporting an error.

44 ↗

(On Diff #113895)

Maybe rephrase the message a little bit, mentioning the name of the flag. Something like:
"Value of --compile-commands-dir must be an absolute path. The argument will be ignored."

[Nebiroth]

Modified CompileCommandsDir to only look in the specified path if the value is set.
Moved CompileCommandsDir field to DirectoryBasedGlobalCompilationDatabase class.
Minor refactoring.

[ilya-biryukov]

Could you also clang-format the code please?

clangd/GlobalCompilationDatabase.cpp

87 ↗

(On Diff #114199)

Why do we use parent_path of CompilerCommandsDir, not CompilerCommandsDir itself?
Why do we need to reassign the File parameter?

Maybe simply do
auto CDB = tryLoadDatabaseFromPath(CompileCommandsDir, Error)?

92 ↗

(On Diff #114199)

Code style: don't use else after return.
See https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return

clangd/tool/ClangdMain.cpp

40 ↗

(On Diff #114199)

Replace --CompileCommandsDir with --compile-commands-dir.

[ilya-biryukov]

clangd/ClangdServer.cpp

150 ↗

(On Diff #114199)

Remove CompileCommandsDir from ClangdServer parameters, it is hanlded by DirectoryBasedGlobalCompilationDatabase.

clangd/GlobalCompilationDatabase.cpp

70 ↗

(On Diff #114199)

Restore this assertion, move it to tryLoadDatabaseFromPath.

84 ↗

(On Diff #114199)

Maybe move this local variable into tryLoadDatabaseFromPath and remove Error parameter from tryLoadDatabaseFromPath?
Please make sure to move the TODO about logging into tryLoadDatabaseFromPath as well.

clangd/GlobalCompilationDatabase.h

48 ↗

(On Diff #114199)

Rename parameter to CompileCommandsDir, change type to llvm::Optional<Path>.

53 ↗

(On Diff #114199)

Make this function private.

clangd/tool/ClangdMain.cpp

45 ↗

(On Diff #114199)

Use Out.log() instead of Logs << for consistency with other logging code.

51 ↗

(On Diff #114199)

Maybe also rephrase to mention the name of the flag? Something like:
"Path specified by --compile-commands-dir does not exist. The argument will be ignored.\n"

58 ↗

(On Diff #114199)

Accept llvm::Optional in ClangdLSPServer, pass llvm::None if CompileCommandsDir == "" here.

[Nebiroth]

Ran clang-format on modified files.
More minor refactoring.

[klimek]

clangd/GlobalCompilationDatabase.cpp

98–100 ↗

(On Diff #114371)

Isn't that the same as "return CDB"?

[Nebiroth]

Simpilified a pointer return value.

[ilya-biryukov]

clangd/GlobalCompilationDatabase.cpp

75 ↗

(On Diff #114654)

Parentheses seem redundant.

78 ↗

(On Diff #114654)

Code style: local variable are UpperCamelCase

96 ↗

(On Diff #114654)

Even better: return tryLoadDatabaseFromPath(CompileCommandsDir);

clangd/GlobalCompilationDatabase.h

50 ↗

(On Diff #114654)

getValue() will fail if NewCompileCommandsDir does not have a value.
The original suggestion was to change type of the field 'CompileCommandsDir` to llvm::Optional<Path> and check if Optional has a value instead of checking for empty string.

58 ↗

(On Diff #114654)

Could you please move this to other fields?

Mixing functions and fields is a bit confusing.

clangd/tool/ClangdMain.cpp

52 ↗

(On Diff #114654)

Message does not mention that path does not exist. Probably incidental.

59 ↗

(On Diff #114654)

Could you please move this out of the logic that validates CompileCommandsDir parameter?
It's very easy to miss while reading the code. Probably ok to put it right after JSONOutput Out(... line

[Nebiroth]

Fixed a few comments.
Rebased on latest clangd version.

[ilya-biryukov]

All lit test for clangd are currently failing if I apply your change.
Generally, please run make check-clang-tools to make sure your changes do not introduce any regressions.

The tests are failing because when --compile-commands-dir is not specified clangd now shows the cmd arg validation errors ("path not absolute", "path does not exist").

clangd/tool/ClangdMain.cpp

78 ↗

(On Diff #115462)

The code doing command line argument validation shows errors using llvm::errs() and not Out.log(). Could you do the same for consistency?

Also, could you put your new code that does argument validation right after the previous checks (to line 65)? So that we have a clear structure in the code:

/// Validate command line arguments.
...

/// Initialize and run ClangdLSPServer.
...

[Nebiroth]

More consistent logging in clangdmain.
Restructured argument checking in ClangdMain
Fixed empty compile-commands-dir triggering error messages.
Fixed failing standard tests.

[ilya-biryukov]

Could you please run clang-format on every submission?

clangd/GlobalCompilationDatabase.cpp

98 ↗

(On Diff #116198)

Is this some kind of accidental change? Why do we need to assign "/" to CompileCommandsDir?

clangd/tool/ClangdMain.cpp

20 ↗

(On Diff #116198)

We certainly don't need that include.

79 ↗

(On Diff #116198)

This should be something like:

if (CompileCommandsDir.empty()) {
  //...
  CompileCommandsDirPath = llvm::None;
} else if (!is_absolute(...)) {
  //....
  CompileCommandsDirPath = llvm::None;
} else if (!exists(...)) {
  // ....
  CompileCommandsDirPath = llvm::None;
} else {
  CompileCommandsDirPath = CompileCommandsDir;
}

• It will have less nesting, therefore making code more readable.
• It will fix an error in the current implementation. (Currently, exists check will run on an empty string if -compile-commands-dir is not an absolute path).

[Nebiroth]

Fixed inverted compile_commands check logic that made tests fail.
More readable command arg checks.

[malaperle]

clangd/tool/ClangdMain.cpp

20 ↗

(On Diff #116559)

William, did you perhaps miss this comment? I don't think unistd.h makes sense here.

[Nebiroth]

clangd/tool/ClangdMain.cpp

20 ↗

(On Diff #116559)

I indeed missed it.

[ilya-biryukov]

clangd/GlobalCompilationDatabase.cpp

82 ↗

(On Diff #116198)

Why remove this FIXME?

clangd/tool/ClangdMain.cpp

86 ↗

(On Diff #116559)

NIT: maybe join this with previous line?

89 ↗

(On Diff #116559)

NIT: don't mix branches with and without {}. Simply use {} on all branches in that case.

[Nebiroth]

Fixed missed comments and suggstions.

[ilya-biryukov]

It seems some changes were be lost while merging with head.

clangd/GlobalCompilationDatabase.cpp

75 ↗

(On Diff #117217)

Maybe move Error closer to its usage (line 84: auto CDB = tooling::CompilationDatabase::loadFromDirectory ...)?

90 ↗

(On Diff #117217)

Please remove this FIXME, it is already deleted in head.

97 ↗

(On Diff #117217)

Please restore this logging statement.

[Nebiroth]

Fixed changes that were lost while merging with head.

[ilya-biryukov]

clangd/GlobalCompilationDatabase.cpp

90 ↗

(On Diff #117327)

This logging statement is misplaced, in the current HEAD it's outside of the loop.
(i.e. it should be in the getCompilationDatabase, not in tryLoadDatabaseFromPath).

Otherwise the output of clangd gets really spammy.

100 ↗

(On Diff #117327)

We should also log if we can't find compilation database in this code path.

[Nebiroth]

Changed placement of logging instruction to reduce logging output.

[ilya-biryukov]

Thanks. LGTM modulo minor NIT (see other inline comment).
Do you need help to land this?

clangd/GlobalCompilationDatabase.cpp

102 ↗

(On Diff #117337)

Message should probably mention that's clangd was looking for compile commands in an overridden directory.

[Nebiroth]

Improved logging message when unable to find compilation database in specified overriden directory.

[ilya-biryukov]

Thanks for fixing the last comment.
Do you want me to land this for you?

clangd/GlobalCompilationDatabase.cpp

103 ↗

(On Diff #117340)

NIT: missing a space at the start of "in overriden...". Otherwise filename will be concatenated with "in"

[Nebiroth]

In D37150#885749, @ilya-biryukov wrote:

Thanks for fixing the last comment.
Do you want me to land this for you?

Yes please!

[ilya-biryukov]

Did a minor rename and added a few std::moves before submitting. Was not worth another round of code review.

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

 namespace clangd {
 
 class JSONOutput;
 
 /// This class provides implementation of an LSP server, glueing the JSON
 /// dispatch and ClangdServer together.
 class ClangdLSPServer : private DiagnosticsConsumer, private ProtocolCallbacks {
 public:
+  /// If \p CompileCommandsDir has a value, compile_commands.json will be
+  /// loaded only from \p CompileCommandsDir. Otherwise, clangd will look
+  /// for compile_commands.json in all parent directories of each file.
   ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount,
                   bool SnippetCompletions,
-                  llvm::Optional<StringRef> ResourceDir);
+                  llvm::Optional<StringRef> ResourceDir,
+                  llvm::Optional<Path> CompileCommandsDir);
 
   /// Run LSP server loop, receiving input for it from \p In. \p In must be
   /// opened in binary mode. Output will be written using Out variable passed to
   /// class constructor. This method must not be executed more than once for
   /// each instance of ClangdLSPServer.
   void run(std::istream &In);
 
 private:

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

void ClangdLSPServer::onSwitchSourceHeader(TextDocumentIdentifier Params,
 
   Out.writeMessage(
       R"({"jsonrpc":"2.0","id":)" + ID.str() +
       R"(,"result":)" + ResultUri + R"(})");
 }
 
 ClangdLSPServer::ClangdLSPServer(JSONOutput &Out, unsigned AsyncThreadsCount,
                                  bool SnippetCompletions,
-                                 llvm::Optional<StringRef> ResourceDir)
-    : Out(Out), CDB(/*Logger=*/Out),
+                                 llvm::Optional<StringRef> ResourceDir,
+                                 llvm::Optional<Path> CompileCommandsDir)
+    : Out(Out), CDB(/*Logger=*/Out, std::move(CompileCommandsDir)),
       Server(CDB, /*DiagConsumer=*/*this, FSProvider, AsyncThreadsCount,
              SnippetCompletions, /*Logger=*/Out, ResourceDir) {}
 
 void ClangdLSPServer::run(std::istream &In) {
   assert(!IsDone && "Run was called before");
 
   // Set up JSONRPCDispatcher.
   JSONRPCDispatcher Dispatcher(llvm::make_unique<Handler>(Out));

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

public:
   /// existing targets.
 };
 
 /// Gets compile args from tooling::CompilationDatabases built for parent
 /// directories.
 class DirectoryBasedGlobalCompilationDatabase
     : public GlobalCompilationDatabase {
 public:
-  DirectoryBasedGlobalCompilationDatabase(clangd::Logger &Logger);
+  DirectoryBasedGlobalCompilationDatabase(
+      clangd::Logger &Logger, llvm::Optional<Path> CompileCommandsDir);
 
   std::vector<tooling::CompileCommand>
   getCompileCommands(PathRef File) override;
 
   void setExtraFlagsForFile(PathRef File, std::vector<std::string> ExtraFlags);
 
 private:
   tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
+  tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File);
 
   std::mutex Mutex;
   /// Caches compilation databases loaded from directories(keys are
   /// directories).
   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

 //===--- GlobalCompilationDatabase.cpp --------------------------*- C++-*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===---------------------------------------------------------------------===//
 
 #include "GlobalCompilationDatabase.h"
+#include "Logger.h"
 #include "clang/Tooling/CompilationDatabase.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
-#include "Logger.h"
 
 namespace clang {
 namespace clangd {
 
 static void addExtraFlags(tooling::CompileCommand &Command,
                           const std::vector<std::string> &ExtraFlags) {
   if (ExtraFlags.empty())
     return;

 tooling::CompileCommand getDefaultCompileCommand(PathRef File) {
   std::vector<std::string> CommandLine{"clang", "-fsyntax-only", File.str()};
   return tooling::CompileCommand(llvm::sys::path::parent_path(File),
                                  llvm::sys::path::filename(File), CommandLine,
                                  /*Output=*/"");
 }
 
 DirectoryBasedGlobalCompilationDatabase::
-    DirectoryBasedGlobalCompilationDatabase(clangd::Logger &Logger)
-    : Logger(Logger) {}
+    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);

 }
 
 void DirectoryBasedGlobalCompilationDatabase::setExtraFlagsForFile(
     PathRef File, std::vector<std::string> ExtraFlags) {
   ExtraFlagsForFile[File] = std::move(ExtraFlags);
 }
 
 tooling::CompilationDatabase *
-DirectoryBasedGlobalCompilationDatabase::getCompilationDatabase(PathRef File) {
-  std::lock_guard<std::mutex> Lock(Mutex);
+DirectoryBasedGlobalCompilationDatabase::tryLoadDatabaseFromPath(PathRef File) {
 
   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");
 
-  for (auto Path = path::parent_path(File); !Path.empty();
-       Path = path::parent_path(Path)) {
-
-    auto CachedIt = CompilationDatabases.find(Path);
-    if (CachedIt != CompilationDatabases.end())
-      return CachedIt->second.get();
+  std::string Error = "";
+  auto CDB = tooling::CompilationDatabase::loadFromDirectory(File, Error);
+  if (CDB && Error.empty()) {
+    auto Result = CDB.get();
+    CompilationDatabases.insert(std::make_pair(File, std::move(CDB)));
+    return Result;
+  }
+
+  return nullptr;
+}
 
-    std::string Error;
-    auto CDB = tooling::CompilationDatabase::loadFromDirectory(Path, Error);
+tooling::CompilationDatabase *
+DirectoryBasedGlobalCompilationDatabase::getCompilationDatabase(PathRef File) {
+  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) +
+                 "in overriden directory " + CompileCommandsDir.getValue() +
+                 "\n");
+    return ReturnValue;
+  }
+
+  for (auto Path = path::parent_path(File); !Path.empty();
+       Path = path::parent_path(Path)) {
+    auto CDB = tryLoadDatabaseFromPath(Path);
     if (!CDB)
       continue;
-
     // FIXME(ibiryukov): Invalidate cached compilation databases on changes
-    auto Result = CDB.get();
-    CompilationDatabases.insert(std::make_pair(Path, std::move(CDB)));
-    return Result;
+    return CDB;
   }
 
   Logger.log("Failed to find compilation database for " + Twine(File) + "\n");
   return nullptr;
 }
 
 } // namespace clangd
 } // namespace clang

clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp

 //===--- ClangdMain.cpp - clangd server loop ------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "ClangdLSPServer.h"
 #include "JSONRPCDispatcher.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
 #include "llvm/Support/Program.h"
-
 #include <iostream>
 #include <memory>
 #include <string>
 #include <thread>
 
 using namespace clang;
 using namespace clang::clangd;
 
+static llvm::cl::opt<Path> CompileCommandsDir(
+    "compile-commands-dir",
+    llvm::cl::desc("Specify a path to look for compile_commands.json. If path "
+                   "is invalid, clangd will look in the current directory and "
+                   "parent paths of each source file."));
+
 static llvm::cl::opt<unsigned>
     WorkerThreadsCount("j",
                        llvm::cl::desc("Number of async workers used by clangd"),
                        llvm::cl::init(getDefaultAsyncThreadsCount()));
 
 static llvm::cl::opt<bool> EnableSnippets(
     "enable-snippets",
     llvm::cl::desc(
if (!RunSynchronously && WorkerThreadsCount == 0) {
     return 1;
   }
 
   // Ignore -j option if -run-synchonously is used.
   // FIXME: a warning should be shown here.
   if (RunSynchronously)
     WorkerThreadsCount = 0;
 
+  /// Validate command line arguments.
   llvm::raw_ostream &Outs = llvm::outs();
   llvm::raw_ostream &Logs = llvm::errs();
   JSONOutput Out(Outs, Logs);
 
-  // Change stdin to binary to not lose \r\n on windows.
-  llvm::sys::ChangeStdinToBinary();
+  // If --compile-commands-dir arg was invoked, check value and override default
+  // path.
+  namespace path = llvm::sys::path;
+  llvm::Optional<Path> CompileCommandsDirPath;
+
+  if (CompileCommandsDir.empty()) {
+    CompileCommandsDirPath = llvm::None;
+  } else if (!llvm::sys::path::is_absolute(CompileCommandsDir) ||
+             !llvm::sys::fs::exists(CompileCommandsDir)) {
+    llvm::errs() << "Path specified by --compile-commands-dir either does not "
+                    "exist or is not an absolute "
+                    "path. The argument will be ignored.\n";
+    CompileCommandsDirPath = llvm::None;
+  } else {
+    CompileCommandsDirPath = CompileCommandsDir;
+  }
 
   llvm::Optional<StringRef> ResourceDirRef = None;
   if (!ResourceDir.empty())
     ResourceDirRef = ResourceDir;
 
+  /// Change stdin to binary to not lose \r\n on windows.
+  llvm::sys::ChangeStdinToBinary();
+
+  /// Initialize and run ClangdLSPServer.
   ClangdLSPServer LSPServer(Out, WorkerThreadsCount, EnableSnippets,
-                            ResourceDirRef);
+                            ResourceDirRef, CompileCommandsDirPath);
   LSPServer.run(std::cin);
 }