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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Could you also `clang-format` the code please?



================
Comment at: clangd/ClangdServer.cpp:150
                            bool RunSynchronously,
+                           llvm::Optional<Path> CompileCommandsDir,
                            llvm::Optional<StringRef> ResourceDir)
----------------
Remove `CompileCommandsDir` from ClangdServer parameters, it is hanlded by `DirectoryBasedGlobalCompilationDatabase`.


================
Comment at: clangd/GlobalCompilationDatabase.cpp:70
-
-  assert((path::is_absolute(File, path::Style::posix) ||
-          path::is_absolute(File, path::Style::windows)) &&
----------------
Restore this assertion, move it to `tryLoadDatabaseFromPath`.


================
Comment at: clangd/GlobalCompilationDatabase.cpp:84
   namespace path = llvm::sys::path;
-
-  assert((path::is_absolute(File, path::Style::posix) ||
-          path::is_absolute(File, path::Style::windows)) &&
-         "path must be absolute");
+  std::string Error;
+  if (!CompileCommandsDir.empty()) {
----------------
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.


================
Comment at: clangd/GlobalCompilationDatabase.cpp:87
+    File = CompileCommandsDir;
+    File = path::parent_path(File);
+    auto CDB = tryLoadDatabaseFromPath(File, Error);
----------------
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)`?


================
Comment at: clangd/GlobalCompilationDatabase.cpp:92
+      return CDB;
+    else
+      return nullptr;
----------------
Code style: don't use `else` after `return`.
See https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return


================
Comment at: clangd/GlobalCompilationDatabase.h:48
 public:
+  DirectoryBasedGlobalCompilationDatabase(Path NewCompileCommandsDir): CompileCommandsDir(NewCompileCommandsDir){}
   std::vector<tooling::CompileCommand>
----------------
Rename parameter to `CompileCommandsDir`, change type to `llvm::Optional<Path>`.


================
Comment at: clangd/GlobalCompilationDatabase.h:53
   void setExtraFlagsForFile(PathRef File, std::vector<std::string> ExtraFlags);
+  tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File, std::string &Error);
 
----------------
Make this function `private`.


================
Comment at: clangd/tool/ClangdMain.cpp:40
 
+  // If --CompileCommandsDir arg was invoked, check value and override default path.
+  namespace path = llvm::sys::path;
----------------
Replace `--CompileCommandsDir` with `--compile-commands-dir`.


================
Comment at: clangd/tool/ClangdMain.cpp:45
+  {
+    Logs << "Path specified by --compile-commands-dir must be an absolute path. The argument will be ignored.\n";
+    CompileCommandsDir = "";
----------------
Use `Out.log()` instead of `Logs <<` for consistency with other logging code.


================
Comment at: clangd/tool/ClangdMain.cpp:51
+  {
+    Logs << "File does not exist. The argument will be ignored.\n";
+    CompileCommandsDir = "";
----------------
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"`


================
Comment at: clangd/tool/ClangdMain.cpp:58
 
-  ClangdLSPServer LSPServer(Out, RunSynchronously);
+  ClangdLSPServer LSPServer(Out, RunSynchronously, CompileCommandsDir);
   LSPServer.run(std::cin);
----------------
Accept `llvm::Optional` in ClangdLSPServer, pass `llvm::None` if `CompileCommandsDir == ""` here.


https://reviews.llvm.org/D37150