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