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

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

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



================
Comment at: clangd/ClangdServer.cpp:150
                            bool RunSynchronously,
+                           std::string CompileCommands,
                            llvm::Optional<StringRef> ResourceDir)
----------------
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()`?


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


================
Comment at: clangd/GlobalCompilationDatabase.cpp:14
 #include "llvm/Support/Path.h"
+#include "llvm/ADT/Statistic.h"
+#include "llvm/ADT/StringExtras.h"
----------------
There are definitely unneeded includes here. Remove most of them and leave only the required ones?


================
Comment at: clangd/GlobalCompilationDatabase.cpp:93
+  namespace path = llvm::sys::path; 
+  if (llvm::sys::fs::exists(CompileCommands))
+    File = CompileCommands;
----------------
Let's simply lookup **only** inside `CompileCommandsDir` if it was set.
Otherwise, fallback into looking at `parent_path`s of the file.


================
Comment at: clangd/GlobalCompilationDatabase.h:38
   getCompileCommands(PathRef File) = 0;
+  std::string CompileCommands;  
 
----------------
`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.




================
Comment at: clangd/tool/ClangdMain.cpp:29
 
+static llvm::cl::opt<std::string>
+    CompileCommands("compile-commands-dir",
----------------
Could you use descriptive `Path`  typedef  instead of `std::string` here and in other places?


================
Comment at: clangd/tool/ClangdMain.cpp:30
+static llvm::cl::opt<std::string>
+    CompileCommands("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.")); 
----------------
Maybe rename this (and other occurences) to `CompileCommandsDir`?


================
Comment at: clangd/tool/ClangdMain.cpp:43
+  namespace path = llvm::sys::path;
+  if (!llvm::sys::path::is_absolute(CompileCommands) || CompileCommands.empty())
+    Logs << "Path specified for compilation database must be absolute. Verifying current folder instead.";
----------------
  - `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.






================
Comment at: clangd/tool/ClangdMain.cpp:44
+  if (!llvm::sys::path::is_absolute(CompileCommands) || CompileCommands.empty())
+    Logs << "Path specified for compilation database must be absolute. Verifying current folder instead.";
+
----------------
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."`


https://reviews.llvm.org/D37150