[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:29
+
+static cl::opt<std::string> CompileCommands("compileCommands",
+                                 cl::desc("Start with absolute path to compile_commands.json"));  
----------------
Please place this flag in `ClangdMain.cpp`.


================
Comment at: clangd/GlobalCompilationDatabase.cpp:29
+
+static cl::opt<std::string> CompileCommands("compileCommands",
+                                 cl::desc("Start with absolute path to compile_commands.json"));  
----------------
ilya-biryukov wrote:
> Please place this flag in `ClangdMain.cpp`.
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`?


================
Comment at: clangd/GlobalCompilationDatabase.cpp:30
+static cl::opt<std::string> CompileCommands("compileCommands",
+                                 cl::desc("Start with absolute path to compile_commands.json"));  
+}
----------------
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?


================
Comment at: clangd/GlobalCompilationDatabase.cpp:57
 
+
+
----------------
Incidental whitespace change?


================
Comment at: clangd/GlobalCompilationDatabase.cpp:89
+  // if --compileCommands arg was invoked, check value and override default path
+  std::size_t found = CompileCommands.find_first_of("/");
+  std::string TempString = CompileCommands;
----------------
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`.


================
Comment at: clangd/GlobalCompilationDatabase.cpp:97
+  std::string Error;
+  bool badPath = false;
+  File = path::parent_path(File);
----------------
Naming: local variables should be `UpperCamelCase`


================
Comment at: clangd/GlobalCompilationDatabase.cpp:99
+  File = path::parent_path(File);
+  auto CachedIt = CompilationDatabases.find(File);
+  if (CachedIt != CompilationDatabases.end())
----------------
Maybe extract a function from the for loop body instead of copying it here?
Could be named `tryLoadDatabaseFromPath`.


================
Comment at: clangd/GlobalCompilationDatabase.cpp:113
 
-  for (auto Path = path::parent_path(File); !Path.empty();
+  if (badPath)
+  {
----------------
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...
```


================
Comment at: clangd/GlobalCompilationDatabase.cpp:138
+  {
+    auto result = CDB.get();
+    CompilationDatabases.insert(std::make_pair(File, std::move(CDB)));
----------------
Please consider removing this code duplication by extracting a function from the loop body (also see my previous comment).


https://reviews.llvm.org/D37150