[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
ilya-biryukov added a comment. Did a minor rename and added a few `std::move`s before submitting. Was not worth another round of code review. Repository: rL LLVM https://reviews.llvm.org/D37150 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
This revision was automatically updated to reflect the committed changes.
Closed by commit rL314678: [clangd] Command line arg to specify
compile_commands.json path (authored by ibiryukov).
Changed prior to commit:
https://reviews.llvm.org/D37150?vs=117340=117354#toc
Repository:
rL LLVM
https://reviews.llvm.org/D37150
Files:
clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
clang-tools-extra/trunk/clangd/ClangdLSPServer.h
clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.cpp
clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h
clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
Index: clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/trunk/clangd/ClangdLSPServer.cpp
@@ -196,8 +196,9 @@
ClangdLSPServer::ClangdLSPServer(JSONOutput , unsigned AsyncThreadsCount,
bool SnippetCompletions,
- llvm::Optional ResourceDir)
-: Out(Out), CDB(/*Logger=*/Out),
+ llvm::Optional ResourceDir,
+ llvm::Optional CompileCommandsDir)
+: Out(Out), CDB(/*Logger=*/Out, std::move(CompileCommandsDir)),
Server(CDB, /*DiagConsumer=*/*this, FSProvider, AsyncThreadsCount,
SnippetCompletions, /*Logger=*/Out, ResourceDir) {}
Index: clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/trunk/clangd/tool/ClangdMain.cpp
@@ -11,16 +11,22 @@
#include "JSONRPCDispatcher.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
-
#include
#include
#include
#include
using namespace clang;
using namespace clang::clangd;
+static llvm::cl::opt CompileCommandsDir(
+"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."));
+
static llvm::cl::opt
WorkerThreadsCount("j",
llvm::cl::desc("Number of async workers used by clangd"),
@@ -56,18 +62,37 @@
if (RunSynchronously)
WorkerThreadsCount = 0;
+ /// Validate command line arguments.
llvm::raw_ostream = llvm::outs();
llvm::raw_ostream = llvm::errs();
JSONOutput Out(Outs, Logs);
- // Change stdin to binary to not lose \r\n on windows.
- llvm::sys::ChangeStdinToBinary();
+ // If --compile-commands-dir arg was invoked, check value and override default
+ // path.
+ namespace path = llvm::sys::path;
+ llvm::Optional CompileCommandsDirPath;
+
+ if (CompileCommandsDir.empty()) {
+CompileCommandsDirPath = llvm::None;
+ } else if (!llvm::sys::path::is_absolute(CompileCommandsDir) ||
+ !llvm::sys::fs::exists(CompileCommandsDir)) {
+llvm::errs() << "Path specified by --compile-commands-dir either does not "
+"exist or is not an absolute "
+"path. The argument will be ignored.\n";
+CompileCommandsDirPath = llvm::None;
+ } else {
+CompileCommandsDirPath = CompileCommandsDir;
+ }
llvm::Optional ResourceDirRef = None;
if (!ResourceDir.empty())
ResourceDirRef = ResourceDir;
+ /// Change stdin to binary to not lose \r\n on windows.
+ llvm::sys::ChangeStdinToBinary();
+
+ /// Initialize and run ClangdLSPServer.
ClangdLSPServer LSPServer(Out, WorkerThreadsCount, EnableSnippets,
-ResourceDirRef);
+ResourceDirRef, CompileCommandsDirPath);
LSPServer.run(std::cin);
}
Index: clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h
===
--- clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h
+++ clang-tools-extra/trunk/clangd/GlobalCompilationDatabase.h
@@ -47,15 +47,17 @@
class DirectoryBasedGlobalCompilationDatabase
: public GlobalCompilationDatabase {
public:
- DirectoryBasedGlobalCompilationDatabase(clangd::Logger );
+ DirectoryBasedGlobalCompilationDatabase(
+ clangd::Logger , llvm::Optional CompileCommandsDir);
std::vector
getCompileCommands(PathRef File) override;
void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
private:
tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
+ tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File);
std::mutex Mutex;
/// Caches compilation databases loaded from directories(keys are
@@ -67,6 +69,9 @@
llvm::StringMap ExtraFlagsForFile;
/// Used for logging.
clangd::Logger
+ /// Used for command argument pointing to folder where
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
Nebiroth added a comment. In https://reviews.llvm.org/D37150#885749, @ilya-biryukov wrote: > Thanks for fixing the last comment. > Do you want me to land this for you? Yes please! https://reviews.llvm.org/D37150 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
ilya-biryukov added a comment.
Thanks for fixing the last comment.
Do you want me to land this for you?
Comment at: clangd/GlobalCompilationDatabase.cpp:103
+ Logger.log("Failed to find compilation database for " + Twine(File) +
+ "in overriden directory " + CompileCommandsDir.getValue() +
+ "\n");
NIT: missing a space at the start of `"in overriden..."`. Otherwise filename
will be concatenated with `"in"`
https://reviews.llvm.org/D37150
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
Nebiroth updated this revision to Diff 117340.
Nebiroth added a comment.
Improved logging message when unable to find compilation database in specified
overriden directory.
https://reviews.llvm.org/D37150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/GlobalCompilationDatabase.cpp
clangd/GlobalCompilationDatabase.h
clangd/tool/ClangdMain.cpp
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -11,16 +11,22 @@
#include "JSONRPCDispatcher.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
-
#include
#include
#include
#include
using namespace clang;
using namespace clang::clangd;
+static llvm::cl::opt CompileCommandsDir(
+"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."));
+
static llvm::cl::opt
WorkerThreadsCount("j",
llvm::cl::desc("Number of async workers used by clangd"),
@@ -56,18 +62,37 @@
if (RunSynchronously)
WorkerThreadsCount = 0;
+ /// Validate command line arguments.
llvm::raw_ostream = llvm::outs();
llvm::raw_ostream = llvm::errs();
JSONOutput Out(Outs, Logs);
- // Change stdin to binary to not lose \r\n on windows.
- llvm::sys::ChangeStdinToBinary();
+ // If --compile-commands-dir arg was invoked, check value and override default
+ // path.
+ namespace path = llvm::sys::path;
+ llvm::Optional CompileCommandsDirPath;
+
+ if (CompileCommandsDir.empty()) {
+CompileCommandsDirPath = llvm::None;
+ } else if (!llvm::sys::path::is_absolute(CompileCommandsDir) ||
+ !llvm::sys::fs::exists(CompileCommandsDir)) {
+llvm::errs() << "Path specified by --compile-commands-dir either does not "
+"exist or is not an absolute "
+"path. The argument will be ignored.\n";
+CompileCommandsDirPath = llvm::None;
+ } else {
+CompileCommandsDirPath = CompileCommandsDir;
+ }
llvm::Optional ResourceDirRef = None;
if (!ResourceDir.empty())
ResourceDirRef = ResourceDir;
+ /// Change stdin to binary to not lose \r\n on windows.
+ llvm::sys::ChangeStdinToBinary();
+
+ /// Initialize and run ClangdLSPServer.
ClangdLSPServer LSPServer(Out, WorkerThreadsCount, EnableSnippets,
-ResourceDirRef);
+ResourceDirRef, CompileCommandsDirPath);
LSPServer.run(std::cin);
}
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -47,15 +47,17 @@
class DirectoryBasedGlobalCompilationDatabase
: public GlobalCompilationDatabase {
public:
- DirectoryBasedGlobalCompilationDatabase(clangd::Logger );
+ DirectoryBasedGlobalCompilationDatabase(
+ clangd::Logger , llvm::Optional NewCompileCommandsDir);
std::vector
getCompileCommands(PathRef File) override;
void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
private:
tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
+ tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File);
std::mutex Mutex;
/// Caches compilation databases loaded from directories(keys are
@@ -67,6 +69,9 @@
llvm::StringMap ExtraFlagsForFile;
/// Used for logging.
clangd::Logger
+ /// Used for command argument pointing to folder where compile_commands.json
+ /// is located.
+ llvm::Optional CompileCommandsDir;
};
} // namespace clangd
} // namespace clang
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -8,10 +8,10 @@
//===-===//
#include "GlobalCompilationDatabase.h"
+#include "Logger.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
-#include "Logger.h"
namespace clang {
namespace clangd {
@@ -38,8 +38,9 @@
}
DirectoryBasedGlobalCompilationDatabase::
-DirectoryBasedGlobalCompilationDatabase(clangd::Logger )
-: Logger(Logger) {}
+DirectoryBasedGlobalCompilationDatabase(
+clangd::Logger , llvm::Optional NewCompileCommandsDir)
+: Logger(Logger), CompileCommandsDir(NewCompileCommandsDir) {}
std::vector
DirectoryBasedGlobalCompilationDatabase::getCompileCommands(PathRef File) {
@@ -67,31 +68,50 @@
}
tooling::CompilationDatabase *
-DirectoryBasedGlobalCompilationDatabase::getCompilationDatabase(PathRef File) {
-
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
Thanks. LGTM modulo minor NIT (see other inline comment).
Do you need help to land this?
Comment at: clangd/GlobalCompilationDatabase.cpp:102
+if (ReturnValue == nullptr)
+ Logger.log("Failed to find compilation database for " + Twine(File) +
+ "\n");
Message should probably mention that's clangd was looking for compile commands
in an overridden directory.
https://reviews.llvm.org/D37150
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
Nebiroth updated this revision to Diff 117337.
Nebiroth marked 2 inline comments as done.
Nebiroth added a comment.
Changed placement of logging instruction to reduce logging output.
https://reviews.llvm.org/D37150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/GlobalCompilationDatabase.cpp
clangd/GlobalCompilationDatabase.h
clangd/tool/ClangdMain.cpp
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -11,16 +11,22 @@
#include "JSONRPCDispatcher.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
-
#include
#include
#include
#include
using namespace clang;
using namespace clang::clangd;
+static llvm::cl::opt CompileCommandsDir(
+"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."));
+
static llvm::cl::opt
WorkerThreadsCount("j",
llvm::cl::desc("Number of async workers used by clangd"),
@@ -56,18 +62,37 @@
if (RunSynchronously)
WorkerThreadsCount = 0;
+ /// Validate command line arguments.
llvm::raw_ostream = llvm::outs();
llvm::raw_ostream = llvm::errs();
JSONOutput Out(Outs, Logs);
- // Change stdin to binary to not lose \r\n on windows.
- llvm::sys::ChangeStdinToBinary();
+ // If --compile-commands-dir arg was invoked, check value and override default
+ // path.
+ namespace path = llvm::sys::path;
+ llvm::Optional CompileCommandsDirPath;
+
+ if (CompileCommandsDir.empty()) {
+CompileCommandsDirPath = llvm::None;
+ } else if (!llvm::sys::path::is_absolute(CompileCommandsDir) ||
+ !llvm::sys::fs::exists(CompileCommandsDir)) {
+llvm::errs() << "Path specified by --compile-commands-dir either does not "
+"exist or is not an absolute "
+"path. The argument will be ignored.\n";
+CompileCommandsDirPath = llvm::None;
+ } else {
+CompileCommandsDirPath = CompileCommandsDir;
+ }
llvm::Optional ResourceDirRef = None;
if (!ResourceDir.empty())
ResourceDirRef = ResourceDir;
+ /// Change stdin to binary to not lose \r\n on windows.
+ llvm::sys::ChangeStdinToBinary();
+
+ /// Initialize and run ClangdLSPServer.
ClangdLSPServer LSPServer(Out, WorkerThreadsCount, EnableSnippets,
-ResourceDirRef);
+ResourceDirRef, CompileCommandsDirPath);
LSPServer.run(std::cin);
}
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -47,15 +47,17 @@
class DirectoryBasedGlobalCompilationDatabase
: public GlobalCompilationDatabase {
public:
- DirectoryBasedGlobalCompilationDatabase(clangd::Logger );
+ DirectoryBasedGlobalCompilationDatabase(
+ clangd::Logger , llvm::Optional NewCompileCommandsDir);
std::vector
getCompileCommands(PathRef File) override;
void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
private:
tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
+ tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File);
std::mutex Mutex;
/// Caches compilation databases loaded from directories(keys are
@@ -67,6 +69,9 @@
llvm::StringMap ExtraFlagsForFile;
/// Used for logging.
clangd::Logger
+ /// Used for command argument pointing to folder where compile_commands.json
+ /// is located.
+ llvm::Optional CompileCommandsDir;
};
} // namespace clangd
} // namespace clang
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -8,10 +8,10 @@
//===-===//
#include "GlobalCompilationDatabase.h"
+#include "Logger.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
-#include "Logger.h"
namespace clang {
namespace clangd {
@@ -38,8 +38,9 @@
}
DirectoryBasedGlobalCompilationDatabase::
-DirectoryBasedGlobalCompilationDatabase(clangd::Logger )
-: Logger(Logger) {}
+DirectoryBasedGlobalCompilationDatabase(
+clangd::Logger , llvm::Optional NewCompileCommandsDir)
+: Logger(Logger), CompileCommandsDir(NewCompileCommandsDir) {}
std::vector
DirectoryBasedGlobalCompilationDatabase::getCompileCommands(PathRef File) {
@@ -67,31 +68,49 @@
}
tooling::CompilationDatabase *
-DirectoryBasedGlobalCompilationDatabase::getCompilationDatabase(PathRef
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
ilya-biryukov requested changes to this revision.
ilya-biryukov added inline comments.
This revision now requires changes to proceed.
Comment at: clangd/GlobalCompilationDatabase.cpp:90
+
+ Logger.log("Failed to find compilation database for " + Twine(File) + "\n");
+ return nullptr;
This logging statement is misplaced, in the current HEAD it's outside of the
loop.
(i.e. it should be in the `getCompilationDatabase`, not in
`tryLoadDatabaseFromPath`).
Otherwise the output of clangd gets really spammy.
Comment at: clangd/GlobalCompilationDatabase.cpp:100
+ if (CompileCommandsDir.hasValue())
+return tryLoadDatabaseFromPath(CompileCommandsDir.getValue());
We should also log if we can't find compilation database in this code path.
https://reviews.llvm.org/D37150
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
Nebiroth updated this revision to Diff 117327.
Nebiroth added a comment.
Fixed changes that were lost while merging with head.
https://reviews.llvm.org/D37150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/GlobalCompilationDatabase.cpp
clangd/GlobalCompilationDatabase.h
clangd/tool/ClangdMain.cpp
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -11,16 +11,22 @@
#include "JSONRPCDispatcher.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
-
#include
#include
#include
#include
using namespace clang;
using namespace clang::clangd;
+static llvm::cl::opt CompileCommandsDir(
+"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."));
+
static llvm::cl::opt
WorkerThreadsCount("j",
llvm::cl::desc("Number of async workers used by clangd"),
@@ -56,18 +62,37 @@
if (RunSynchronously)
WorkerThreadsCount = 0;
+ /// Validate command line arguments.
llvm::raw_ostream = llvm::outs();
llvm::raw_ostream = llvm::errs();
JSONOutput Out(Outs, Logs);
- // Change stdin to binary to not lose \r\n on windows.
- llvm::sys::ChangeStdinToBinary();
+ // If --compile-commands-dir arg was invoked, check value and override default
+ // path.
+ namespace path = llvm::sys::path;
+ llvm::Optional CompileCommandsDirPath;
+
+ if (CompileCommandsDir.empty()) {
+CompileCommandsDirPath = llvm::None;
+ } else if (!llvm::sys::path::is_absolute(CompileCommandsDir) ||
+ !llvm::sys::fs::exists(CompileCommandsDir)) {
+llvm::errs() << "Path specified by --compile-commands-dir either does not "
+"exist or is not an absolute "
+"path. The argument will be ignored.\n";
+CompileCommandsDirPath = llvm::None;
+ } else {
+CompileCommandsDirPath = CompileCommandsDir;
+ }
llvm::Optional ResourceDirRef = None;
if (!ResourceDir.empty())
ResourceDirRef = ResourceDir;
+ /// Change stdin to binary to not lose \r\n on windows.
+ llvm::sys::ChangeStdinToBinary();
+
+ /// Initialize and run ClangdLSPServer.
ClangdLSPServer LSPServer(Out, WorkerThreadsCount, EnableSnippets,
-ResourceDirRef);
+ResourceDirRef, CompileCommandsDirPath);
LSPServer.run(std::cin);
}
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -47,15 +47,17 @@
class DirectoryBasedGlobalCompilationDatabase
: public GlobalCompilationDatabase {
public:
- DirectoryBasedGlobalCompilationDatabase(clangd::Logger );
+ DirectoryBasedGlobalCompilationDatabase(
+ clangd::Logger , llvm::Optional NewCompileCommandsDir);
std::vector
getCompileCommands(PathRef File) override;
void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
private:
tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
+ tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File);
std::mutex Mutex;
/// Caches compilation databases loaded from directories(keys are
@@ -67,6 +69,9 @@
llvm::StringMap ExtraFlagsForFile;
/// Used for logging.
clangd::Logger
+ /// Used for command argument pointing to folder where compile_commands.json
+ /// is located.
+ llvm::Optional CompileCommandsDir;
};
} // namespace clangd
} // namespace clang
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -8,10 +8,10 @@
//===-===//
#include "GlobalCompilationDatabase.h"
+#include "Logger.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
-#include "Logger.h"
namespace clang {
namespace clangd {
@@ -38,8 +38,9 @@
}
DirectoryBasedGlobalCompilationDatabase::
-DirectoryBasedGlobalCompilationDatabase(clangd::Logger )
-: Logger(Logger) {}
+DirectoryBasedGlobalCompilationDatabase(
+clangd::Logger , llvm::Optional NewCompileCommandsDir)
+: Logger(Logger), CompileCommandsDir(NewCompileCommandsDir) {}
std::vector
DirectoryBasedGlobalCompilationDatabase::getCompileCommands(PathRef File) {
@@ -67,34 +68,46 @@
}
tooling::CompilationDatabase *
-DirectoryBasedGlobalCompilationDatabase::getCompilationDatabase(PathRef File) {
- std::lock_guard Lock(Mutex);
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.
Some changes seem to be lost while merging with head.
Comment at: clangd/GlobalCompilationDatabase.cpp:75
+ auto CachedIt = CompilationDatabases.find(File);
+ std::string Error = "";
Maybe move `Error` closer to its usage (line 84: `auto CDB =
tooling::CompilationDatabase::loadFromDirectory ...`)?
Comment at: clangd/GlobalCompilationDatabase.cpp:90
+
+ // FIXME(ibiryukov): logging
+ // Output.log("Failed to find compilation database for " + Twine(File) +
Please remove this FIXME, it is already deleted in head.
Comment at: clangd/GlobalCompilationDatabase.cpp:97
- Logger.log("Failed to find compilation database for " + Twine(File) + "\n");
return nullptr;
Please restore this logging statement.
https://reviews.llvm.org/D37150
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
Nebiroth updated this revision to Diff 117217.
Nebiroth added a comment.
Fixed missed comments and suggstions.
https://reviews.llvm.org/D37150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/GlobalCompilationDatabase.cpp
clangd/GlobalCompilationDatabase.h
clangd/tool/ClangdMain.cpp
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -11,16 +11,22 @@
#include "JSONRPCDispatcher.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
-
#include
#include
#include
#include
using namespace clang;
using namespace clang::clangd;
+static llvm::cl::opt CompileCommandsDir(
+"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."));
+
static llvm::cl::opt
WorkerThreadsCount("j",
llvm::cl::desc("Number of async workers used by clangd"),
@@ -56,18 +62,37 @@
if (RunSynchronously)
WorkerThreadsCount = 0;
+ /// Validate command line arguments.
llvm::raw_ostream = llvm::outs();
llvm::raw_ostream = llvm::errs();
JSONOutput Out(Outs, Logs);
- // Change stdin to binary to not lose \r\n on windows.
- llvm::sys::ChangeStdinToBinary();
+ // If --compile-commands-dir arg was invoked, check value and override default
+ // path.
+ namespace path = llvm::sys::path;
+ llvm::Optional CompileCommandsDirPath;
+
+ if (CompileCommandsDir.empty()) {
+CompileCommandsDirPath = llvm::None;
+ } else if (!llvm::sys::path::is_absolute(CompileCommandsDir) ||
+ !llvm::sys::fs::exists(CompileCommandsDir)) {
+llvm::errs() << "Path specified by --compile-commands-dir either does not "
+"exist or is not an absolute "
+"path. The argument will be ignored.\n";
+CompileCommandsDirPath = llvm::None;
+ } else {
+CompileCommandsDirPath = CompileCommandsDir;
+ }
llvm::Optional ResourceDirRef = None;
if (!ResourceDir.empty())
ResourceDirRef = ResourceDir;
+ /// Change stdin to binary to not lose \r\n on windows.
+ llvm::sys::ChangeStdinToBinary();
+
+ /// Initialize and run ClangdLSPServer.
ClangdLSPServer LSPServer(Out, WorkerThreadsCount, EnableSnippets,
-ResourceDirRef);
+ResourceDirRef, CompileCommandsDirPath);
LSPServer.run(std::cin);
}
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -47,15 +47,17 @@
class DirectoryBasedGlobalCompilationDatabase
: public GlobalCompilationDatabase {
public:
- DirectoryBasedGlobalCompilationDatabase(clangd::Logger );
+ DirectoryBasedGlobalCompilationDatabase(
+ clangd::Logger , llvm::Optional NewCompileCommandsDir);
std::vector
getCompileCommands(PathRef File) override;
void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
private:
tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
+ tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File);
std::mutex Mutex;
/// Caches compilation databases loaded from directories(keys are
@@ -67,6 +69,9 @@
llvm::StringMap ExtraFlagsForFile;
/// Used for logging.
clangd::Logger
+ /// Used for command argument pointing to folder where compile_commands.json
+ /// is located.
+ llvm::Optional CompileCommandsDir;
};
} // namespace clangd
} // namespace clang
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -8,10 +8,10 @@
//===-===//
#include "GlobalCompilationDatabase.h"
+#include "Logger.h"
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
-#include "Logger.h"
namespace clang {
namespace clangd {
@@ -38,8 +38,9 @@
}
DirectoryBasedGlobalCompilationDatabase::
-DirectoryBasedGlobalCompilationDatabase(clangd::Logger )
-: Logger(Logger) {}
+DirectoryBasedGlobalCompilationDatabase(
+clangd::Logger , llvm::Optional NewCompileCommandsDir)
+: Logger(Logger), CompileCommandsDir(NewCompileCommandsDir) {}
std::vector
DirectoryBasedGlobalCompilationDatabase::getCompileCommands(PathRef File) {
@@ -67,34 +68,48 @@
}
tooling::CompilationDatabase *
-DirectoryBasedGlobalCompilationDatabase::getCompilationDatabase(PathRef File) {
- std::lock_guard Lock(Mutex);
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
ilya-biryukov added inline comments.
Comment at: clangd/GlobalCompilationDatabase.cpp:82
// FIXME(ibiryukov): Invalidate cached compilation databases on changes
+return Result;
Why remove this `FIXME`?
Comment at: clangd/tool/ClangdMain.cpp:86
+"The argument will be "
+"ignored.\n";
+CompileCommandsDirPath = llvm::None;
NIT: maybe join this with previous line?
Comment at: clangd/tool/ClangdMain.cpp:89
+ } else
+CompileCommandsDirPath = CompileCommandsDir;
NIT: don't mix branches with and without `{}`. Simply use `{}` on all branches
in that case.
https://reviews.llvm.org/D37150
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
Nebiroth added inline comments. Comment at: clangd/tool/ClangdMain.cpp:20 #include +#include malaperle wrote: > William, did you perhaps miss this comment? I don't think unistd.h makes > sense here. I indeed missed it. https://reviews.llvm.org/D37150 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
malaperle requested changes to this revision. malaperle added inline comments. This revision now requires changes to proceed. Comment at: clangd/tool/ClangdMain.cpp:20 #include +#include William, did you perhaps miss this comment? I don't think unistd.h makes sense 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
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
Nebiroth updated this revision to Diff 116559.
Nebiroth added a comment.
Fixed inverted compile_commands check logic that made tests fail.
More readable command arg checks.
https://reviews.llvm.org/D37150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/GlobalCompilationDatabase.cpp
clangd/GlobalCompilationDatabase.h
clangd/tool/ClangdMain.cpp
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -11,16 +11,23 @@
#include "JSONRPCDispatcher.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
-
#include
#include
#include
#include
+#include
using namespace clang;
using namespace clang::clangd;
+static llvm::cl::opt CompileCommandsDir(
+"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."));
+
static llvm::cl::opt
WorkerThreadsCount("j",
llvm::cl::desc("Number of async workers used by clangd"),
@@ -56,18 +63,40 @@
if (RunSynchronously)
WorkerThreadsCount = 0;
+ /// Validate command line arguments.
llvm::raw_ostream = llvm::outs();
llvm::raw_ostream = llvm::errs();
JSONOutput Out(Outs, Logs);
- // Change stdin to binary to not lose \r\n on windows.
- llvm::sys::ChangeStdinToBinary();
+ // If --compile-commands-dir arg was invoked, check value and override default
+ // path.
+ namespace path = llvm::sys::path;
+ llvm::Optional CompileCommandsDirPath;
+
+ if (CompileCommandsDir.empty())
+CompileCommandsDirPath = llvm::None;
+ else if (!llvm::sys::path::is_absolute(CompileCommandsDir)) {
+llvm::errs()
+<< "Path specified by --compile-commands-dir must be an absolute "
+ "path. The argument will be ignored.\n";
+CompileCommandsDirPath = llvm::None;
+ } else if (!llvm::sys::fs::exists(CompileCommandsDir)) {
+llvm::errs() << "Path specified by --compile-commands-dir does not exist. "
+"The argument will be "
+"ignored.\n";
+CompileCommandsDirPath = llvm::None;
+ } else
+CompileCommandsDirPath = CompileCommandsDir;
llvm::Optional ResourceDirRef = None;
if (!ResourceDir.empty())
ResourceDirRef = ResourceDir;
+ /// Change stdin to binary to not lose \r\n on windows.
+ llvm::sys::ChangeStdinToBinary();
+
+ /// Initialize and run ClangdLSPServer.
ClangdLSPServer LSPServer(Out, WorkerThreadsCount, EnableSnippets,
-ResourceDirRef);
+ResourceDirRef, CompileCommandsDirPath);
LSPServer.run(std::cin);
}
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -47,15 +47,17 @@
class DirectoryBasedGlobalCompilationDatabase
: public GlobalCompilationDatabase {
public:
- DirectoryBasedGlobalCompilationDatabase(clangd::Logger );
+ DirectoryBasedGlobalCompilationDatabase(
+ clangd::Logger , llvm::Optional NewCompileCommandsDir);
std::vector
getCompileCommands(PathRef File) override;
void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
private:
tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
+ tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File);
std::mutex Mutex;
/// Caches compilation databases loaded from directories(keys are
@@ -67,6 +69,9 @@
llvm::StringMap ExtraFlagsForFile;
/// Used for logging.
clangd::Logger
+ /// Used for command argument pointing to folder where compile_commands.json
+ /// is located.
+ llvm::Optional CompileCommandsDir;
};
} // namespace clangd
} // namespace clang
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -38,8 +38,9 @@
}
DirectoryBasedGlobalCompilationDatabase::
-DirectoryBasedGlobalCompilationDatabase(clangd::Logger )
-: Logger(Logger) {}
+DirectoryBasedGlobalCompilationDatabase(
+clangd::Logger , llvm::Optional NewCompileCommandsDir)
+: Logger(Logger), CompileCommandsDir(NewCompileCommandsDir) {}
std::vector
DirectoryBasedGlobalCompilationDatabase::getCompileCommands(PathRef File) {
@@ -67,34 +68,48 @@
}
tooling::CompilationDatabase *
-DirectoryBasedGlobalCompilationDatabase::getCompilationDatabase(PathRef File) {
- std::lock_guard Lock(Mutex);
+DirectoryBasedGlobalCompilationDatabase::tryLoadDatabaseFromPath(PathRef File) {
namespace path = llvm::sys::path;
+ auto CachedIt =
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
ilya-biryukov added a comment.
Could you please run `clang-format` on every submission?
Comment at: clangd/GlobalCompilationDatabase.cpp:98
+ {
+CompileCommandsDir = "/";
+return tryLoadDatabaseFromPath(CompileCommandsDir.getValue());
Is this some kind of accidental change? Why do we need to assign `"/"` to
`CompileCommandsDir`?
Comment at: clangd/tool/ClangdMain.cpp:20
#include
+#include
We certainly don't need that include.
Comment at: clangd/tool/ClangdMain.cpp:79
+CompileCommandsDirPath = llvm::None;
+ else
+ {
This should be something like:
```
if (CompileCommandsDir.empty()) {
//...
CompileCommandsDirPath = llvm::None;
} else if (!is_absolute(...)) {
//
CompileCommandsDirPath = llvm::None;
} else if (!exists(...)) {
//
CompileCommandsDirPath = llvm::None;
} else {
CompileCommandsDirPath = CompileCommandsDir;
}
```
- It will have less nesting, therefore making code more readable.
- It will fix an error in the current implementation. (Currently, `exists`
check will run on an empty string if `-compile-commands-dir` is not an absolute
path).
https://reviews.llvm.org/D37150
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
Nebiroth updated this revision to Diff 116198.
Nebiroth added a comment.
More consistent logging in clangdmain.
Restructured argument checking in ClangdMain
Fixed empty compile-commands-dir triggering error messages.
Fixed failing standard tests.
https://reviews.llvm.org/D37150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/GlobalCompilationDatabase.cpp
clangd/GlobalCompilationDatabase.h
clangd/tool/ClangdMain.cpp
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -11,16 +11,24 @@
#include "JSONRPCDispatcher.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
-
#include
#include
#include
#include
+#include
using namespace clang;
using namespace clang::clangd;
+static llvm::cl::opt CompileCommandsDir(
+"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."));
+
+
static llvm::cl::opt
WorkerThreadsCount("j",
llvm::cl::desc("Number of async workers used by clangd"),
@@ -56,18 +64,44 @@
if (RunSynchronously)
WorkerThreadsCount = 0;
+ /// Validate command line arguments.
llvm::raw_ostream = llvm::outs();
llvm::raw_ostream = llvm::errs();
- JSONOutput Out(Outs, Logs);
+ JSONOutput Out(Outs, Logs);
- // Change stdin to binary to not lose \r\n on windows.
- llvm::sys::ChangeStdinToBinary();
+ // If --compile-commands-dir arg was invoked, check value and override default
+ // path.
+ namespace path = llvm::sys::path;
+ llvm::Optional CompileCommandsDirPath;
+
+ if (CompileCommandsDir.empty())
+CompileCommandsDirPath = llvm::None;
+ else
+ {
+if (!llvm::sys::path::is_absolute(CompileCommandsDir)) {
+ llvm::errs() << "Path specified by --compile-commands-dir must be an absolute "
+ "path. The argument will be ignored.\n";
+ CompileCommandsDir = "";
+}
+
+if (!llvm::sys::fs::exists(CompileCommandsDir)) {
+ llvm::errs() << "Path specified by --compile-commands-dir does not exist. The argument will be "
+ "ignored.\n";
+ CompileCommandsDir = "";
+}
+CompileCommandsDirPath = CompileCommandsDir;
+ }
llvm::Optional ResourceDirRef = None;
if (!ResourceDir.empty())
ResourceDirRef = ResourceDir;
+ /// Change stdin to binary to not lose \r\n on windows.
+ llvm::sys::ChangeStdinToBinary();
+
+ /// Initialize and run ClangdLSPServer.
+
ClangdLSPServer LSPServer(Out, WorkerThreadsCount, EnableSnippets,
-ResourceDirRef);
+ResourceDirRef, CompileCommandsDirPath);
LSPServer.run(std::cin);
}
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -45,14 +45,19 @@
class DirectoryBasedGlobalCompilationDatabase
: public GlobalCompilationDatabase {
public:
+ DirectoryBasedGlobalCompilationDatabase(
+ llvm::Optional NewCompileCommandsDir)
+ : CompileCommandsDir(NewCompileCommandsDir) {}
std::vector
getCompileCommands(PathRef File) override;
void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
private:
- tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
+ tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
+ tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File);
+ llvm::Optional CompileCommandsDir;
std::mutex Mutex;
/// Caches compilation databases loaded from directories(keys are
/// directories).
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -62,43 +62,54 @@
}
tooling::CompilationDatabase *
-DirectoryBasedGlobalCompilationDatabase::getCompilationDatabase(PathRef File) {
- std::lock_guard Lock(Mutex);
-
+DirectoryBasedGlobalCompilationDatabase::tryLoadDatabaseFromPath(PathRef File) {
+
namespace path = llvm::sys::path;
+ auto CachedIt = CompilationDatabases.find(File);
+ std::string Error = "";
assert((path::is_absolute(File, path::Style::posix) ||
path::is_absolute(File, path::Style::windows)) &&
"path must be absolute");
- for (auto Path = path::parent_path(File); !Path.empty();
- Path = path::parent_path(Path)) {
-
-auto CachedIt = CompilationDatabases.find(Path);
-if (CachedIt != CompilationDatabases.end())
- return CachedIt->second.get();
-std::string Error;
-auto CDB =
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
ilya-biryukov added a comment.
All lit test for clangd are currently failing if I apply your change.
Generally, please run `make check-clang-tools` to make sure your changes do not
introduce any regressions.
The tests are failing because when `--compile-commands-dir` is not specified
clangd now shows the cmd arg validation errors (`"path not absolute"`, `"path
does not exist"`).
Comment at: clangd/tool/ClangdMain.cpp:78
+ if (!llvm::sys::path::is_absolute(CompileCommandsDir)) {
+Out.log("Path specified by --compile-commands-dir must be an absolute "
+"path. The argument will be ignored.\n");
The code doing command line argument validation shows errors using
`llvm::errs()` and not `Out.log()`. Could you do the same for consistency?
Also, could you put your new code that does argument validation right after the
previous checks (to line 65)? So that we have a clear structure in the code:
```
/// Validate command line arguments.
...
/// Initialize and run ClangdLSPServer.
...
```
https://reviews.llvm.org/D37150
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
Nebiroth updated this revision to Diff 115462.
Nebiroth marked an inline comment as done.
Nebiroth added a comment.
Fixed a few comments.
Rebased on latest clangd version.
https://reviews.llvm.org/D37150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/GlobalCompilationDatabase.cpp
clangd/GlobalCompilationDatabase.h
clangd/tool/ClangdMain.cpp
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -11,16 +11,23 @@
#include "JSONRPCDispatcher.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
-
#include
#include
#include
#include
using namespace clang;
using namespace clang::clangd;
+static llvm::cl::opt CompileCommandsDir(
+"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."));
+
+
static llvm::cl::opt
WorkerThreadsCount("j",
llvm::cl::desc("Number of async workers used by clangd"),
@@ -59,15 +66,37 @@
llvm::raw_ostream = llvm::outs();
llvm::raw_ostream = llvm::errs();
JSONOutput Out(Outs, Logs);
-
+
// Change stdin to binary to not lose \r\n on windows.
llvm::sys::ChangeStdinToBinary();
+ // If --compile-commands-dir arg was invoked, check value and override default
+ // path.
+ namespace path = llvm::sys::path;
+
+ if (!llvm::sys::path::is_absolute(CompileCommandsDir)) {
+Out.log("Path specified by --compile-commands-dir must be an absolute "
+"path. The argument will be ignored.\n");
+CompileCommandsDir = "";
+ }
+
+ if (!llvm::sys::fs::exists(CompileCommandsDir)) {
+Out.log("Path specified by --compile-commands-dir does not exist. The argument will be "
+"ignored.\n");
+CompileCommandsDir = "";
+ }
+ llvm::Optional CompileCommandsDirPath;
+
+ if (CompileCommandsDir.empty())
+CompileCommandsDirPath = llvm::None;
+ else
+CompileCommandsDirPath = CompileCommandsDir;
+
llvm::Optional ResourceDirRef = None;
if (!ResourceDir.empty())
ResourceDirRef = ResourceDir;
ClangdLSPServer LSPServer(Out, WorkerThreadsCount, EnableSnippets,
-ResourceDirRef);
+ResourceDirRef, CompileCommandsDirPath);
LSPServer.run(std::cin);
}
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -45,14 +45,19 @@
class DirectoryBasedGlobalCompilationDatabase
: public GlobalCompilationDatabase {
public:
+ DirectoryBasedGlobalCompilationDatabase(
+ llvm::Optional NewCompileCommandsDir)
+ : CompileCommandsDir(NewCompileCommandsDir) {}
std::vector
getCompileCommands(PathRef File) override;
void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
private:
- tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
+ tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
+ tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File);
+ llvm::Optional CompileCommandsDir;
std::mutex Mutex;
/// Caches compilation databases loaded from directories(keys are
/// directories).
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -62,43 +62,50 @@
}
tooling::CompilationDatabase *
-DirectoryBasedGlobalCompilationDatabase::getCompilationDatabase(PathRef File) {
- std::lock_guard Lock(Mutex);
-
+DirectoryBasedGlobalCompilationDatabase::tryLoadDatabaseFromPath(PathRef File) {
namespace path = llvm::sys::path;
+ auto CachedIt = CompilationDatabases.find(File);
+ std::string Error = "";
assert((path::is_absolute(File, path::Style::posix) ||
path::is_absolute(File, path::Style::windows)) &&
"path must be absolute");
- for (auto Path = path::parent_path(File); !Path.empty();
- Path = path::parent_path(Path)) {
-
-auto CachedIt = CompilationDatabases.find(Path);
-if (CachedIt != CompilationDatabases.end())
- return CachedIt->second.get();
-std::string Error;
-auto CDB = tooling::CompilationDatabase::loadFromDirectory(Path, Error);
-if (!CDB) {
- if (!Error.empty()) {
-// FIXME(ibiryukov): logging
-// Output.log("Error when trying to load compilation database from " +
-//Twine(Path) + ": " + Twine(Error) + "\n");
- }
- continue;
-}
+ if (CachedIt != CompilationDatabases.end())
+return CachedIt->second.get();
+ auto CDB =
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
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 NewCompileCommandsDir)
+ : CompileCommandsDir(NewCompileCommandsDir.getValue()) {}
std::vector
`getValue()` will fail if NewCompileCommandsDir does not have a value.
The original suggestion was to change type of the field 'CompileCommandsDir` to
`llvm::Optional` 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
Nebiroth updated this revision to Diff 114654.
Nebiroth added a comment.
Simpilified a pointer return value.
https://reviews.llvm.org/D37150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/GlobalCompilationDatabase.cpp
clangd/GlobalCompilationDatabase.h
clangd/tool/ClangdMain.cpp
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -11,8 +11,8 @@
#include "JSONRPCDispatcher.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
-
#include
#include
#include
@@ -25,16 +25,43 @@
llvm::cl::desc("parse on main thread"),
llvm::cl::init(false), llvm::cl::Hidden);
+static llvm::cl::opt CompileCommandsDir(
+"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."));
+
int main(int argc, char *argv[]) {
llvm::cl::ParseCommandLineOptions(argc, argv, "clangd");
llvm::raw_ostream = llvm::outs();
llvm::raw_ostream = llvm::errs();
JSONOutput Out(Outs, Logs);
+ // If --compile-commands-dir arg was invoked, check value and override default
+ // path.
+ namespace path = llvm::sys::path;
+
+ if (!llvm::sys::path::is_absolute(CompileCommandsDir)) {
+Out.log("Path specified by --compile-commands-dir must be an absolute "
+"path. The argument will be ignored.\n");
+CompileCommandsDir = "";
+ }
+
+ if (!llvm::sys::fs::exists(CompileCommandsDir)) {
+Out.log("Path specified by --compile-commands-dir. The argument will be "
+"ignored.\n");
+CompileCommandsDir = "";
+ }
+ llvm::Optional CompileCommandsDirPath;
+
// Change stdin to binary to not lose \r\n on windows.
llvm::sys::ChangeStdinToBinary();
+ if (CompileCommandsDir.empty())
+CompileCommandsDirPath = llvm::None;
+ else
+CompileCommandsDirPath = CompileCommandsDir;
- ClangdLSPServer LSPServer(Out, RunSynchronously);
+ ClangdLSPServer LSPServer(Out, RunSynchronously, CompileCommandsDirPath);
LSPServer.run(std::cin);
}
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -45,13 +45,18 @@
class DirectoryBasedGlobalCompilationDatabase
: public GlobalCompilationDatabase {
public:
+ DirectoryBasedGlobalCompilationDatabase(
+ llvm::Optional NewCompileCommandsDir)
+ : CompileCommandsDir(NewCompileCommandsDir.getValue()) {}
std::vector
getCompileCommands(PathRef File) override;
void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
private:
tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
+ Path CompileCommandsDir;
+ tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File);
std::mutex Mutex;
/// Caches compilation databases loaded from directories(keys are
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -62,43 +62,52 @@
}
tooling::CompilationDatabase *
-DirectoryBasedGlobalCompilationDatabase::getCompilationDatabase(PathRef File) {
- std::lock_guard Lock(Mutex);
-
+DirectoryBasedGlobalCompilationDatabase::tryLoadDatabaseFromPath(PathRef File) {
namespace path = llvm::sys::path;
+ auto CachedIt = CompilationDatabases.find(File);
+ std::string Error = "";
assert((path::is_absolute(File, path::Style::posix) ||
path::is_absolute(File, path::Style::windows)) &&
"path must be absolute");
- for (auto Path = path::parent_path(File); !Path.empty();
- Path = path::parent_path(Path)) {
-
-auto CachedIt = CompilationDatabases.find(Path);
-if (CachedIt != CompilationDatabases.end())
- return CachedIt->second.get();
-std::string Error;
-auto CDB = tooling::CompilationDatabase::loadFromDirectory(Path, Error);
-if (!CDB) {
- if (!Error.empty()) {
-// FIXME(ibiryukov): logging
-// Output.log("Error when trying to load compilation database from " +
-//Twine(Path) + ": " + Twine(Error) + "\n");
- }
- continue;
-}
+ if (CachedIt != CompilationDatabases.end())
+return (CachedIt->second.get());
+ auto CDB = tooling::CompilationDatabase::loadFromDirectory(File, Error);
+ if (CDB && Error.empty()) {
+auto result = CDB.get();
+CompilationDatabases.insert(std::make_pair(File, std::move(CDB)));
// FIXME(ibiryukov): Invalidate cached compilation databases on changes
-auto result = CDB.get();
-
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
klimek added inline comments. Comment at: clangd/GlobalCompilationDatabase.cpp:98-100 +if (CDB) + return CDB; +return nullptr; Isn't that the same as "return CDB"? https://reviews.llvm.org/D37150 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
Nebiroth updated this revision to Diff 114371.
Nebiroth marked 10 inline comments as done.
Nebiroth added a comment.
Ran clang-format on modified files.
More minor refactoring.
https://reviews.llvm.org/D37150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/GlobalCompilationDatabase.cpp
clangd/GlobalCompilationDatabase.h
clangd/tool/ClangdMain.cpp
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -11,8 +11,8 @@
#include "JSONRPCDispatcher.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Path.h"
#include "llvm/Support/Program.h"
-
#include
#include
#include
@@ -25,16 +25,43 @@
llvm::cl::desc("parse on main thread"),
llvm::cl::init(false), llvm::cl::Hidden);
+static llvm::cl::opt CompileCommandsDir(
+"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."));
+
int main(int argc, char *argv[]) {
llvm::cl::ParseCommandLineOptions(argc, argv, "clangd");
llvm::raw_ostream = llvm::outs();
llvm::raw_ostream = llvm::errs();
JSONOutput Out(Outs, Logs);
+ // If --compile-commands-dir arg was invoked, check value and override default
+ // path.
+ namespace path = llvm::sys::path;
+
+ if (!llvm::sys::path::is_absolute(CompileCommandsDir)) {
+Out.log("Path specified by --compile-commands-dir must be an absolute "
+"path. The argument will be ignored.\n");
+CompileCommandsDir = "";
+ }
+
+ if (!llvm::sys::fs::exists(CompileCommandsDir)) {
+Out.log("Path specified by --compile-commands-dir. The argument will be "
+"ignored.\n");
+CompileCommandsDir = "";
+ }
+ llvm::Optional CompileCommandsDirPath;
+
// Change stdin to binary to not lose \r\n on windows.
llvm::sys::ChangeStdinToBinary();
+ if (CompileCommandsDir.empty())
+CompileCommandsDirPath = llvm::None;
+ else
+CompileCommandsDirPath = CompileCommandsDir;
- ClangdLSPServer LSPServer(Out, RunSynchronously);
+ ClangdLSPServer LSPServer(Out, RunSynchronously, CompileCommandsDirPath);
LSPServer.run(std::cin);
}
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -45,13 +45,18 @@
class DirectoryBasedGlobalCompilationDatabase
: public GlobalCompilationDatabase {
public:
+ DirectoryBasedGlobalCompilationDatabase(
+ llvm::Optional NewCompileCommandsDir)
+ : CompileCommandsDir(NewCompileCommandsDir.getValue()) {}
std::vector
getCompileCommands(PathRef File) override;
void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
private:
tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
+ Path CompileCommandsDir;
+ tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File);
std::mutex Mutex;
/// Caches compilation databases loaded from directories(keys are
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -62,43 +62,55 @@
}
tooling::CompilationDatabase *
-DirectoryBasedGlobalCompilationDatabase::getCompilationDatabase(PathRef File) {
- std::lock_guard Lock(Mutex);
-
+DirectoryBasedGlobalCompilationDatabase::tryLoadDatabaseFromPath(PathRef File) {
namespace path = llvm::sys::path;
+ auto CachedIt = CompilationDatabases.find(File);
+ std::string Error = "";
assert((path::is_absolute(File, path::Style::posix) ||
path::is_absolute(File, path::Style::windows)) &&
"path must be absolute");
- for (auto Path = path::parent_path(File); !Path.empty();
- Path = path::parent_path(Path)) {
-
-auto CachedIt = CompilationDatabases.find(Path);
-if (CachedIt != CompilationDatabases.end())
- return CachedIt->second.get();
-std::string Error;
-auto CDB = tooling::CompilationDatabase::loadFromDirectory(Path, Error);
-if (!CDB) {
- if (!Error.empty()) {
-// FIXME(ibiryukov): logging
-// Output.log("Error when trying to load compilation database from " +
-//Twine(Path) + ": " + Twine(Error) + "\n");
- }
- continue;
-}
+ if (CachedIt != CompilationDatabases.end())
+return (CachedIt->second.get());
+ auto CDB = tooling::CompilationDatabase::loadFromDirectory(File, Error);
+ if (CDB && Error.empty()) {
+auto result = CDB.get();
+CompilationDatabases.insert(std::make_pair(File, std::move(CDB)));
// FIXME(ibiryukov): Invalidate cached compilation
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
ilya-biryukov added a comment.
Could you also `clang-format` the code please?
Comment at: clangd/ClangdServer.cpp:150
bool RunSynchronously,
+ llvm::Optional CompileCommandsDir,
llvm::Optional 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
Rename parameter to `CompileCommandsDir`, change type to `llvm::Optional`.
Comment at: clangd/GlobalCompilationDatabase.h:53
void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
+ tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File,
std::string );
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
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
Nebiroth updated this revision to Diff 114199.
Nebiroth marked 7 inline comments as done.
Nebiroth added a comment.
Modified CompileCommandsDir to only look in the specified path if the value is
set.
Moved CompileCommandsDir field to DirectoryBasedGlobalCompilationDatabase class.
Minor refactoring.
https://reviews.llvm.org/D37150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/GlobalCompilationDatabase.cpp
clangd/GlobalCompilationDatabase.h
clangd/tool/ClangdMain.cpp
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -12,7 +12,7 @@
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Program.h"
-
+#include "llvm/Support/Path.h"
#include
#include
#include
@@ -25,16 +25,36 @@
llvm::cl::desc("parse on main thread"),
llvm::cl::init(false), llvm::cl::Hidden);
+static llvm::cl::opt
+CompileCommandsDir("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."));
+
+
int main(int argc, char *argv[]) {
llvm::cl::ParseCommandLineOptions(argc, argv, "clangd");
llvm::raw_ostream = llvm::outs();
llvm::raw_ostream = llvm::errs();
JSONOutput Out(Outs, Logs);
+ // If --CompileCommandsDir arg was invoked, check value and override default path.
+ namespace path = llvm::sys::path;
+
+ if (!llvm::sys::path::is_absolute(CompileCommandsDir))
+ {
+Logs << "Path specified by --compile-commands-dir must be an absolute path. The argument will be ignored.\n";
+CompileCommandsDir = "";
+ }
+
+ if (!llvm::sys::fs::exists(CompileCommandsDir))
+ {
+Logs << "File does not exist. The argument will be ignored.\n";
+CompileCommandsDir = "";
+ }
+
// Change stdin to binary to not lose \r\n on windows.
llvm::sys::ChangeStdinToBinary();
- ClangdLSPServer LSPServer(Out, RunSynchronously);
+ ClangdLSPServer LSPServer(Out, RunSynchronously, CompileCommandsDir);
LSPServer.run(std::cin);
}
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -33,9 +33,9 @@
public:
virtual ~GlobalCompilationDatabase() = default;
- virtual std::vector
+ virtual std::vector
getCompileCommands(PathRef File) = 0;
-
+
/// FIXME(ibiryukov): add facilities to track changes to compilation flags of
/// existing targets.
};
@@ -45,13 +45,16 @@
class DirectoryBasedGlobalCompilationDatabase
: public GlobalCompilationDatabase {
public:
+ DirectoryBasedGlobalCompilationDatabase(Path NewCompileCommandsDir): CompileCommandsDir(NewCompileCommandsDir){}
std::vector
getCompileCommands(PathRef File) override;
void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
+ tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File, std::string );
private:
tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
+ Path CompileCommandsDir;
std::mutex Mutex;
/// Caches compilation databases loaded from directories(keys are
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -61,37 +61,47 @@
ExtraFlagsForFile[File] = std::move(ExtraFlags);
}
+tooling::CompilationDatabase * DirectoryBasedGlobalCompilationDatabase::tryLoadDatabaseFromPath(PathRef File, std::string )
+{
+ auto CachedIt = CompilationDatabases.find(File);
+ if (CachedIt != CompilationDatabases.end())
+return (CachedIt->second.get());
+ auto CDB = tooling::CompilationDatabase::loadFromDirectory(File, Error);
+ if (CDB && Error.empty())
+ {
+auto result = CDB.get();
+CompilationDatabases.insert(std::make_pair(File, std::move(CDB)));
+return result;
+ }
+ return nullptr;
+}
+
tooling::CompilationDatabase *
DirectoryBasedGlobalCompilationDatabase::getCompilationDatabase(PathRef File) {
std::lock_guard Lock(Mutex);
-
+
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()) {
+File = CompileCommandsDir;
+File = path::parent_path(File);
+auto CDB = tryLoadDatabaseFromPath(File, Error);
+
+if (CDB && Error.empty())
+ return CDB;
+else
+ return nullptr;
+ }
for (auto Path = path::parent_path(File); !Path.empty();
- Path = path::parent_path(Path)) {
-
-auto CachedIt =
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
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 ResourceDir)
Could you pass `llvm::Optional` 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 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
+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
+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
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
Nebiroth updated this revision to Diff 113895.
Nebiroth added a comment.
Added function to avoid code reuse and extra nesting.
https://reviews.llvm.org/D37150
Files:
clangd/ClangdLSPServer.cpp
clangd/ClangdLSPServer.h
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/GlobalCompilationDatabase.cpp
clangd/GlobalCompilationDatabase.h
clangd/tool/ClangdMain.cpp
test/clangd/hover.test
Index: test/clangd/hover.test
===
--- /dev/null
+++ test/clangd/hover.test
@@ -0,0 +1,26 @@
+# RUN: clangd -run-synchronously < %s | FileCheck %s
+# It is absolutely vital that this file has CRLF line endings.
+#
+Content-Length: 125
+
+{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
+
+Content-Length: 172
+
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"file:///main.cpp","languageId":"cpp","version":1,"text":"int main() {\nint a;\na;\n}\n"}}}
+
+Content-Length: 143
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":0,"character":5}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":{"contents": {"language": "C++", "value": "int main() {\nint a;\na;\n}"}, "range": {"start": {"line": 0, "character": 0}, "end": {"line": 3, "character": 1
+
+Content-Length: 143
+
+{"jsonrpc":"2.0","id":1,"method":"textDocument/hover","params":{"textDocument":{"uri":"file:///main.cpp"},"position":{"line":1,"character":5}}}
+# Go to local variable
+# CHECK: {"jsonrpc":"2.0","id":1,"result":{"contents": {"language": "C++", "value": "int a"}, "range": {"start": {"line": 1, "character": 0}, "end": {"line": 1, "character": 5
+
+Content-Length: 44
+
+{"jsonrpc":"2.0","id":3,"method":"shutdown"}
Index: clangd/tool/ClangdMain.cpp
===
--- clangd/tool/ClangdMain.cpp
+++ clangd/tool/ClangdMain.cpp
@@ -12,6 +12,7 @@
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Program.h"
+#include "llvm/Support/Path.h"
#include
#include
@@ -25,16 +26,29 @@
llvm::cl::desc("parse on main thread"),
llvm::cl::init(false), llvm::cl::Hidden);
+static llvm::cl::opt
+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."));
+
+
int main(int argc, char *argv[]) {
llvm::cl::ParseCommandLineOptions(argc, argv, "clangd");
llvm::raw_ostream = llvm::outs();
llvm::raw_ostream = llvm::errs();
JSONOutput Out(Outs, Logs);
+ // If --compileCommands arg was invoked, check value and override default path.
+ 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.";
+
+ if (!llvm::sys::fs::exists(CompileCommands))
+Logs << "File does not exist. Verifying current folder instead.";
+
// Change stdin to binary to not lose \r\n on windows.
llvm::sys::ChangeStdinToBinary();
- ClangdLSPServer LSPServer(Out, RunSynchronously);
+ ClangdLSPServer LSPServer(Out, RunSynchronously, CompileCommands);
LSPServer.run(std::cin);
}
Index: clangd/GlobalCompilationDatabase.h
===
--- clangd/GlobalCompilationDatabase.h
+++ clangd/GlobalCompilationDatabase.h
@@ -33,8 +33,9 @@
public:
virtual ~GlobalCompilationDatabase() = default;
- virtual std::vector
+ virtual std::vector
getCompileCommands(PathRef File) = 0;
+ std::string CompileCommands;
/// FIXME(ibiryukov): add facilities to track changes to compilation flags of
/// existing targets.
@@ -49,6 +50,7 @@
getCompileCommands(PathRef File) override;
void setExtraFlagsForFile(PathRef File, std::vector ExtraFlags);
+ tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File, std::string );
private:
tooling::CompilationDatabase *getCompilationDatabase(PathRef File);
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -11,6 +11,16 @@
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
+#include "llvm/ADT/Statistic.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Compiler.h"
+#include "llvm/Support/Debug.h"
+#include "llvm/Support/Format.h"
+#include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/Timer.h"
+#include
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
krasimir added inline comments.
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;
ilya-biryukov wrote:
> 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`.
Please make `CompileCommands` a member variable and pass it through the
constructor of `DirectoryBasedGlobalCompilationDatabase`. This is more flexible
and, among other things, allows for easier unit testing.
Comment at: clangd/GlobalCompilationDatabase.cpp:94
+ File = TempString;
+ }
+
LLVM style uses no braces for single statement `if`'s.
https://reviews.llvm.org/D37150
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
ilya-biryukov added a comment. BTW, if you're only looking for providing extra compilation flags for some files, you could use an extension to LSP that is already in clangd source tree. (See tests from https://reviews.llvm.org/D34947 for more details). But your change is also useful, as it allows to override the whole compilation database. Would be interested in this change to land regardless of whether the extra flags thing suits you well. https://reviews.llvm.org/D37150 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
ilya-biryukov added inline comments.
Comment at: clangd/GlobalCompilationDatabase.cpp:29
+
+static cl::opt 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 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 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D37150: [clangd] Command line arg to specify compile_commands.json path
Nebiroth created this revision.
Adds compileCommands command line argument to specify an absolute path directly
to the requested compile_commands.json for flags.
https://reviews.llvm.org/D37150
Files:
clangd/GlobalCompilationDatabase.cpp
Index: clangd/GlobalCompilationDatabase.cpp
===
--- clangd/GlobalCompilationDatabase.cpp
+++ clangd/GlobalCompilationDatabase.cpp
@@ -11,6 +11,24 @@
#include "clang/Tooling/CompilationDatabase.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
+#include "llvm/ADT/Statistic.h"
+#include "llvm/ADT/StringExtras.h"
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/Compiler.h"
+#include "llvm/Support/Debug.h"
+#include "llvm/Support/Format.h"
+#include "llvm/Support/ManagedStatic.h"
+#include "llvm/Support/Mutex.h"
+#include "llvm/Support/Timer.h"
+#include "llvm/Support/YAMLTraits.h"
+#include "llvm/Support/raw_ostream.h"
+using namespace llvm;
+
+namespace {
+
+static cl::opt CompileCommands("compileCommands",
+ cl::desc("Start with absolute path to compile_commands.json"));
+}
namespace clang {
namespace clangd {
@@ -36,6 +54,8 @@
/*Output=*/"");
}
+
+
std::vector
DirectoryBasedGlobalCompilationDatabase::getCompileCommands(PathRef File) {
std::vector Commands;
@@ -64,35 +84,62 @@
tooling::CompilationDatabase *
DirectoryBasedGlobalCompilationDatabase::getCompilationDatabase(PathRef File) {
std::lock_guard Lock(Mutex);
-
namespace path = llvm::sys::path;
+ // if --compileCommands arg was invoked, check value and override default path
+ std::size_t found = CompileCommands.find_first_of("/");
+ std::string TempString = CompileCommands;
+ if (found != std::string::npos)
+ {
+ File = TempString;
+ }
+
+ std::string Error;
+ bool badPath = false;
+ File = path::parent_path(File);
+ auto CachedIt = CompilationDatabases.find(File);
+ if (CachedIt != CompilationDatabases.end())
+return CachedIt->second.get();
+ auto CDB = tooling::CompilationDatabase::loadFromDirectory(File, Error);
+ if (!CDB) {
+ if (!Error.empty()) {
+badPath = true;
+ }
+ }
assert((path::is_absolute(File, path::Style::posix) ||
path::is_absolute(File, path::Style::windows)) &&
"path must be absolute");
- for (auto Path = path::parent_path(File); !Path.empty();
+ if (badPath)
+ {
+for (auto Path = path::parent_path(File); !Path.empty();
Path = path::parent_path(Path)) {
auto CachedIt = CompilationDatabases.find(Path);
if (CachedIt != CompilationDatabases.end())
return CachedIt->second.get();
-std::string Error;
auto CDB = tooling::CompilationDatabase::loadFromDirectory(Path, Error);
if (!CDB) {
if (!Error.empty()) {
// FIXME(ibiryukov): logging
// Output.log("Error when trying to load compilation database from " +
//Twine(Path) + ": " + Twine(Error) + "\n");
- }
- continue;
+ }
}
// FIXME(ibiryukov): Invalidate cached compilation databases on changes
auto result = CDB.get();
CompilationDatabases.insert(std::make_pair(Path, std::move(CDB)));
return result;
+}
}
+ else
+ {
+auto result = CDB.get();
+CompilationDatabases.insert(std::make_pair(File, std::move(CDB)));
+return result;
+ }
+
// FIXME(ibiryukov): logging
// Output.log("Failed to find compilation database for " + Twine(File) +
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
