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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/GlobalCompilationDatabase.cpp:75
+  if (CachedIt != CompilationDatabases.end())
+    return (CachedIt->second.get());
+  auto CDB = tooling::CompilationDatabase::loadFromDirectory(File, Error);
----------------
Parentheses seem redundant.


================
Comment at: clangd/GlobalCompilationDatabase.cpp:78
+  if (CDB && Error.empty()) {
+    auto result = CDB.get();
+    CompilationDatabases.insert(std::make_pair(File, std::move(CDB)));
----------------
Code style: local variable are `UpperCamelCase`


================
Comment at: clangd/GlobalCompilationDatabase.cpp:96
+  if (!CompileCommandsDir.empty()) {
+    auto CDB = tryLoadDatabaseFromPath(CompileCommandsDir);    
+    return CDB;    
----------------
Even better: `return tryLoadDatabaseFromPath(CompileCommandsDir); `


================
Comment at: clangd/GlobalCompilationDatabase.h:50
+      llvm::Optional<Path> NewCompileCommandsDir)
+      : CompileCommandsDir(NewCompileCommandsDir.getValue()) {}
   std::vector<tooling::CompileCommand>
----------------
`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.


================
Comment at: clangd/GlobalCompilationDatabase.h:58
   tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
+  Path CompileCommandsDir;
+  tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File);
----------------
Could you please move this to other fields?

Mixing functions and fields is a bit confusing.


================
Comment at: clangd/tool/ClangdMain.cpp:52
+  if (!llvm::sys::fs::exists(CompileCommandsDir)) {
+    Out.log("Path specified by --compile-commands-dir. The argument will be "
+            "ignored.\n");
----------------
Message does not mention that path does not exist. Probably incidental.


================
Comment at: clangd/tool/ClangdMain.cpp:59
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();
+  if (CompileCommandsDir.empty())
----------------
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


https://reviews.llvm.org/D37150