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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
