[PATCH] D93600: When querying drivers by binary, look in PATH too
rapgenic created this revision.
Herald added subscribers: usaxena95, kadircet, arphaman.
rapgenic requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Sometimes compile_commands.json databases are created without an
absolute path for the driver in the command field. By default the driver
name is appended to the current directory, however if no driver is found
in that location assume it was in the default PATH and try finding it
there
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D93600
Files:
clang-tools-extra/clangd/QueryDriverDatabase.cpp
clang-tools-extra/clangd/test/system-include-extractor.test
Index: clang-tools-extra/clangd/test/system-include-extractor.test
===
--- clang-tools-extra/clangd/test/system-include-extractor.test
+++ clang-tools-extra/clangd/test/system-include-extractor.test
@@ -3,21 +3,24 @@
# The mock driver below is a shell script:
# REQUIRES: shell
+# Create a bin directory to store the mock-driver and add it to the path
+# RUN: mkdir -p %t.dir/bin
+# RUN: export PATH=%t.dir/bin:$PATH
# Generate a mock-driver that will print %temp_dir%/my/dir and
# %temp_dir%/my/dir2 as include search paths.
-# RUN: echo '#!/bin/sh' >> %t.dir/my_driver.sh
-# RUN: echo '[ "$0" = "%t.dir/my_driver.sh" ] || exit' >> %t.dir/my_driver.sh
-# RUN: echo 'args="$*"' >> %t.dir/my_driver.sh
-# RUN: echo '[ -z "${args##*"-nostdinc"*}" ] || exit' >> %t.dir/my_driver.sh
-# RUN: echo '[ -z "${args##*"-isysroot=/isysroot"*}" ] || exit' >>
%t.dir/my_driver.sh
-# RUN: echo 'echo " $* " | grep " --sysroot /my/sysroot/path " || exit' >>
%t.dir/my_driver.sh
-# RUN: echo 'echo line to ignore >&2' >> %t.dir/my_driver.sh
-# RUN: echo 'printf "Target: arm-linux-gnueabihf\r\n" >&2' >>
%t.dir/my_driver.sh
-# RUN: echo 'printf "#include <...> search starts here:\r\n" >&2' >>
%t.dir/my_driver.sh
-# RUN: echo 'echo %t.dir/my/dir/ >&2' >> %t.dir/my_driver.sh
-# RUN: echo 'echo %t.dir/my/dir2/ >&2' >> %t.dir/my_driver.sh
-# RUN: echo 'printf "End of search list.\r\n" >&2' >> %t.dir/my_driver.sh
-# RUN: chmod +x %t.dir/my_driver.sh
+# RUN: echo '#!/bin/sh' >> %t.dir/bin/my_driver.sh
+# RUN: echo '[ "$0" = "%t.dir/bin/my_driver.sh" ] || exit' >>
%t.dir/bin/my_driver.sh
+# RUN: echo 'args="$*"' >> %t.dir/bin/my_driver.sh
+# RUN: echo '[ -z "${args##*"-nostdinc"*}" ] || exit' >>
%t.dir/bin/my_driver.sh
+# RUN: echo '[ -z "${args##*"-isysroot=/isysroot"*}" ] || exit' >>
%t.dir/bin/my_driver.sh
+# RUN: echo 'echo " $* " | grep " --sysroot /my/sysroot/path " || exit' >>
%t.dir/bin/my_driver.sh
+# RUN: echo 'echo line to ignore >&2' >> %t.dir/bin/my_driver.sh
+# RUN: echo 'printf "Target: arm-linux-gnueabihf\r\n" >&2' >>
%t.dir/bin/my_driver.sh
+# RUN: echo 'printf "#include <...> search starts here:\r\n" >&2' >>
%t.dir/bin/my_driver.sh
+# RUN: echo 'echo %t.dir/my/dir/ >&2' >> %t.dir/bin/my_driver.sh
+# RUN: echo 'echo %t.dir/my/dir2/ >&2' >> %t.dir/bin/my_driver.sh
+# RUN: echo 'printf "End of search list.\r\n" >&2' >> %t.dir/bin/my_driver.sh
+# RUN: chmod +x %t.dir/bin/my_driver.sh
# Create header files my/dir/a.h and my/dir2/b.h
# RUN: mkdir -p %t.dir/my/dir
@@ -27,7 +30,7 @@
# Generate a compile_commands.json that will query the mock driver we've
# created. Which should add a.h and b.h into include search path.
-# RUN: echo '[{"directory": "%/t.dir", "command": "%/t.dir/my_driver.sh
the-file.cpp -nostdinc --sysroot /my/sysroot/path -isysroot=/isysroot", "file":
"the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir", "command": "my_driver.sh the-file.cpp
-nostdinc --sysroot /my/sysroot/path -isysroot=/isysroot", "file":
"the-file.cpp"}]' > %t.dir/compile_commands.json
# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
Index: clang-tools-extra/clangd/QueryDriverDatabase.cpp
===
--- clang-tools-extra/clangd/QueryDriverDatabase.cpp
+++ clang-tools-extra/clangd/QueryDriverDatabase.cpp
@@ -334,6 +334,16 @@
llvm::SmallString<128> Driver(Cmd->CommandLine.front());
llvm::sys::fs::make_absolute(Cmd->Directory, Driver);
+if (!llvm::sys::fs::exists(Driver)) {
+ auto DriverProgram =
+ llvm::sys::findProgramByName(Cmd->CommandLine.front());
+ if (DriverProgram) {
+log("System include extraction: driver {0} expanded to {1}",
+Cmd->CommandLine.front(), *DriverProgram);
+Driver = *DriverProgram;
+ }
+}
+
if (auto Info =
QueriedDrivers.get(/*Key=*/(Driver + ":" + Lang).str(), [&] {
return extractSystemIncludesAndTarget(
Index: clang-tools-extra/clangd/test/system-include-extractor.test
===
--- clang-tools-extra/clangd/test/system-include
[PATCH] D93600: [clangd] When querying drivers by binary, look in PATH too
rapgenic added a comment. Just wanted to say that this is my first patch submission to the LLVM/clangd project, so please call me out on any mistake! The arc tool caught me by surprise by opening the revision before I could double check it, hence the rename and edits... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93600/new/ https://reviews.llvm.org/D93600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D93600: [clangd] When querying drivers by binary, look in PATH too
rapgenic updated this revision to Diff 313259.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93600/new/
https://reviews.llvm.org/D93600
Files:
clang-tools-extra/clangd/QueryDriverDatabase.cpp
clang-tools-extra/clangd/test/system-include-extractor.test
Index: clang-tools-extra/clangd/test/system-include-extractor.test
===
--- clang-tools-extra/clangd/test/system-include-extractor.test
+++ clang-tools-extra/clangd/test/system-include-extractor.test
@@ -3,21 +3,24 @@
# The mock driver below is a shell script:
# REQUIRES: shell
+# Create a bin directory to store the mock-driver and add it to the path
+# RUN: mkdir -p %t.dir/bin
+# RUN: export PATH=%t.dir/bin:$PATH
# Generate a mock-driver that will print %temp_dir%/my/dir and
# %temp_dir%/my/dir2 as include search paths.
-# RUN: echo '#!/bin/sh' >> %t.dir/my_driver.sh
-# RUN: echo '[ "$0" = "%t.dir/my_driver.sh" ] || exit' >> %t.dir/my_driver.sh
-# RUN: echo 'args="$*"' >> %t.dir/my_driver.sh
-# RUN: echo '[ -z "${args##*"-nostdinc"*}" ] || exit' >> %t.dir/my_driver.sh
-# RUN: echo '[ -z "${args##*"-isysroot=/isysroot"*}" ] || exit' >>
%t.dir/my_driver.sh
-# RUN: echo 'echo " $* " | grep " --sysroot /my/sysroot/path " || exit' >>
%t.dir/my_driver.sh
-# RUN: echo 'echo line to ignore >&2' >> %t.dir/my_driver.sh
-# RUN: echo 'printf "Target: arm-linux-gnueabihf\r\n" >&2' >>
%t.dir/my_driver.sh
-# RUN: echo 'printf "#include <...> search starts here:\r\n" >&2' >>
%t.dir/my_driver.sh
-# RUN: echo 'echo %t.dir/my/dir/ >&2' >> %t.dir/my_driver.sh
-# RUN: echo 'echo %t.dir/my/dir2/ >&2' >> %t.dir/my_driver.sh
-# RUN: echo 'printf "End of search list.\r\n" >&2' >> %t.dir/my_driver.sh
-# RUN: chmod +x %t.dir/my_driver.sh
+# RUN: echo '#!/bin/sh' >> %t.dir/bin/my_driver.sh
+# RUN: echo '[ "$0" = "%t.dir/bin/my_driver.sh" ] || exit' >>
%t.dir/bin/my_driver.sh
+# RUN: echo 'args="$*"' >> %t.dir/bin/my_driver.sh
+# RUN: echo '[ -z "${args##*"-nostdinc"*}" ] || exit' >>
%t.dir/bin/my_driver.sh
+# RUN: echo '[ -z "${args##*"-isysroot=/isysroot"*}" ] || exit' >>
%t.dir/bin/my_driver.sh
+# RUN: echo 'echo " $* " | grep " --sysroot /my/sysroot/path " || exit' >>
%t.dir/bin/my_driver.sh
+# RUN: echo 'echo line to ignore >&2' >> %t.dir/bin/my_driver.sh
+# RUN: echo 'printf "Target: arm-linux-gnueabihf\r\n" >&2' >>
%t.dir/bin/my_driver.sh
+# RUN: echo 'printf "#include <...> search starts here:\r\n" >&2' >>
%t.dir/bin/my_driver.sh
+# RUN: echo 'echo %t.dir/my/dir/ >&2' >> %t.dir/bin/my_driver.sh
+# RUN: echo 'echo %t.dir/my/dir2/ >&2' >> %t.dir/bin/my_driver.sh
+# RUN: echo 'printf "End of search list.\r\n" >&2' >> %t.dir/bin/my_driver.sh
+# RUN: chmod +x %t.dir/bin/my_driver.sh
# Create header files my/dir/a.h and my/dir2/b.h
# RUN: mkdir -p %t.dir/my/dir
@@ -27,7 +30,7 @@
# Generate a compile_commands.json that will query the mock driver we've
# created. Which should add a.h and b.h into include search path.
-# RUN: echo '[{"directory": "%/t.dir", "command": "%/t.dir/my_driver.sh
the-file.cpp -nostdinc --sysroot /my/sysroot/path -isysroot=/isysroot", "file":
"the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir", "command": "my_driver.sh the-file.cpp
-nostdinc --sysroot /my/sysroot/path -isysroot=/isysroot", "file":
"the-file.cpp"}]' > %t.dir/compile_commands.json
# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
Index: clang-tools-extra/clangd/QueryDriverDatabase.cpp
===
--- clang-tools-extra/clangd/QueryDriverDatabase.cpp
+++ clang-tools-extra/clangd/QueryDriverDatabase.cpp
@@ -136,10 +136,21 @@
}
llvm::Optional
-extractSystemIncludesAndTarget(PathRef Driver, llvm::StringRef Lang,
+extractSystemIncludesAndTarget(llvm::SmallString<128> Driver,
+ llvm::StringRef Lang,
llvm::ArrayRef CommandLine,
const llvm::Regex &QueryDriverRegex) {
trace::Span Tracer("Extract system includes and target");
+
+ if (!llvm::sys::path::is_absolute(Driver)) {
+auto DriverProgram = llvm::sys::findProgramByName(Driver);
+if (DriverProgram) {
+ vlog("System include extraction: driver {0} expanded to {1}", Driver,
+ *DriverProgram);
+ Driver = *DriverProgram;
+}
+ }
+
SPAN_ATTACH(Tracer, "driver", Driver);
SPAN_ATTACH(Tracer, "lang", Lang);
@@ -332,7 +343,11 @@
}
llvm::SmallString<128> Driver(Cmd->CommandLine.front());
-llvm::sys::fs::make_absolute(Cmd->Directory, Driver);
+if (!llvm::none_of(Driver,
+ [](char C) { return llvm::sys::path::is_separator(C);
}))
+ // Driver is a not a single executable name but instead a path (either
+ // relative or absolute).
+ llvm::sys::fs::make_absolute(Cmd->Directory, Driver);
i
[PATCH] D93600: [clangd] When querying drivers by binary, look in PATH too
rapgenic marked 3 inline comments as done. rapgenic added a comment. I think I have fixed what you asked! About the test, I thought it would be reasonable to test the three cases separately, however I cannot get it to work with a single test file (I think it's some problem with the generated files). Should I duplicate it or is there any way to do it better? Sorry for my trivial question, but I'm still learning to navigate the codebase... CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93600/new/ https://reviews.llvm.org/D93600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D93600: [clangd] When querying drivers by binary, look in PATH too
rapgenic updated this revision to Diff 313528.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93600/new/
https://reviews.llvm.org/D93600
Files:
clang-tools-extra/clangd/QueryDriverDatabase.cpp
clang-tools-extra/clangd/test/system-include-extractor.test
Index: clang-tools-extra/clangd/test/system-include-extractor.test
===
--- clang-tools-extra/clangd/test/system-include-extractor.test
+++ clang-tools-extra/clangd/test/system-include-extractor.test
@@ -3,21 +3,24 @@
# The mock driver below is a shell script:
# REQUIRES: shell
+# Create a bin directory to store the mock-driver and add it to the path
+# RUN: mkdir -p %t.dir/bin
+# RUN: export PATH=%t.dir/bin:$PATH
# Generate a mock-driver that will print %temp_dir%/my/dir and
# %temp_dir%/my/dir2 as include search paths.
-# RUN: echo '#!/bin/sh' >> %t.dir/my_driver.sh
-# RUN: echo '[ "$0" = "%t.dir/my_driver.sh" ] || exit' >> %t.dir/my_driver.sh
-# RUN: echo 'args="$*"' >> %t.dir/my_driver.sh
-# RUN: echo '[ -z "${args##*"-nostdinc"*}" ] || exit' >> %t.dir/my_driver.sh
-# RUN: echo '[ -z "${args##*"-isysroot=/isysroot"*}" ] || exit' >>
%t.dir/my_driver.sh
-# RUN: echo 'echo " $* " | grep " --sysroot /my/sysroot/path " || exit' >>
%t.dir/my_driver.sh
-# RUN: echo 'echo line to ignore >&2' >> %t.dir/my_driver.sh
-# RUN: echo 'printf "Target: arm-linux-gnueabihf\r\n" >&2' >>
%t.dir/my_driver.sh
-# RUN: echo 'printf "#include <...> search starts here:\r\n" >&2' >>
%t.dir/my_driver.sh
-# RUN: echo 'echo %t.dir/my/dir/ >&2' >> %t.dir/my_driver.sh
-# RUN: echo 'echo %t.dir/my/dir2/ >&2' >> %t.dir/my_driver.sh
-# RUN: echo 'printf "End of search list.\r\n" >&2' >> %t.dir/my_driver.sh
-# RUN: chmod +x %t.dir/my_driver.sh
+# RUN: echo '#!/bin/sh' >> %t.dir/bin/my_driver.sh
+# RUN: echo '[ "$0" = "%t.dir/bin/my_driver.sh" ] || exit' >>
%t.dir/bin/my_driver.sh
+# RUN: echo 'args="$*"' >> %t.dir/bin/my_driver.sh
+# RUN: echo '[ -z "${args##*"-nostdinc"*}" ] || exit' >>
%t.dir/bin/my_driver.sh
+# RUN: echo '[ -z "${args##*"-isysroot=/isysroot"*}" ] || exit' >>
%t.dir/bin/my_driver.sh
+# RUN: echo 'echo " $* " | grep " --sysroot /my/sysroot/path " || exit' >>
%t.dir/bin/my_driver.sh
+# RUN: echo 'echo line to ignore >&2' >> %t.dir/bin/my_driver.sh
+# RUN: echo 'printf "Target: arm-linux-gnueabihf\r\n" >&2' >>
%t.dir/bin/my_driver.sh
+# RUN: echo 'printf "#include <...> search starts here:\r\n" >&2' >>
%t.dir/bin/my_driver.sh
+# RUN: echo 'echo %t.dir/my/dir/ >&2' >> %t.dir/bin/my_driver.sh
+# RUN: echo 'echo %t.dir/my/dir2/ >&2' >> %t.dir/bin/my_driver.sh
+# RUN: echo 'printf "End of search list.\r\n" >&2' >> %t.dir/bin/my_driver.sh
+# RUN: chmod +x %t.dir/bin/my_driver.sh
# Create header files my/dir/a.h and my/dir2/b.h
# RUN: mkdir -p %t.dir/my/dir
@@ -27,7 +30,7 @@
# Generate a compile_commands.json that will query the mock driver we've
# created. Which should add a.h and b.h into include search path.
-# RUN: echo '[{"directory": "%/t.dir", "command": "%/t.dir/my_driver.sh
the-file.cpp -nostdinc --sysroot /my/sysroot/path -isysroot=/isysroot", "file":
"the-file.cpp"}]' > %t.dir/compile_commands.json
+# RUN: echo '[{"directory": "%/t.dir", "command": "my_driver.sh the-file.cpp
-nostdinc --sysroot /my/sysroot/path -isysroot=/isysroot", "file":
"the-file.cpp"}]' > %t.dir/compile_commands.json
# RUN: sed -e "s|INPUT_DIR|%/t.dir|g" %s > %t.test.1
# On Windows, we need the URI in didOpen to look like "uri":"file:///C:/..."
Index: clang-tools-extra/clangd/QueryDriverDatabase.cpp
===
--- clang-tools-extra/clangd/QueryDriverDatabase.cpp
+++ clang-tools-extra/clangd/QueryDriverDatabase.cpp
@@ -136,10 +136,26 @@
}
llvm::Optional
-extractSystemIncludesAndTarget(PathRef Driver, llvm::StringRef Lang,
+extractSystemIncludesAndTarget(llvm::SmallString<128> Driver,
+ llvm::StringRef Lang,
llvm::ArrayRef CommandLine,
const llvm::Regex &QueryDriverRegex) {
trace::Span Tracer("Extract system includes and target");
+
+ if (!llvm::sys::path::is_absolute(Driver)) {
+assert(llvm::none_of(
+Driver, [](char C) { return llvm::sys::path::is_separator(C); }));
+auto DriverProgram = llvm::sys::findProgramByName(Driver);
+if (DriverProgram) {
+ vlog("System include extraction: driver {0} expanded to {1}", Driver,
+ *DriverProgram);
+ Driver = *DriverProgram;
+} else {
+ elog("System include extraction: driver {0} not found in PATH", Driver);
+ return llvm::None;
+}
+ }
+
SPAN_ATTACH(Tracer, "driver", Driver);
SPAN_ATTACH(Tracer, "lang", Lang);
@@ -332,7 +348,11 @@
}
llvm::SmallString<128> Driver(Cmd->CommandLine.front());
-llvm::sys::fs::make_absolute(Cmd->Directory, Driver);
+if (llvm::any_of(Driver,
+ [](char C) { retur
[PATCH] D93600: [clangd] When querying drivers by binary, look in PATH too
rapgenic marked 3 inline comments as done. rapgenic added a comment. Done! My email is: Giulio Girardi Thank you for your help! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93600/new/ https://reviews.llvm.org/D93600 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D93763: [clangd] Add a flag to disable the documentLinks LSP request
rapgenic created this revision.
Herald added subscribers: usaxena95, kadircet, arphaman.
rapgenic requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang.
On some editors (namely VSCode) documentLinks are rendered with an
underline, which might be distracting to see. This patch adds a flag to
disable such request, as it's used only for includes, and the open include
feature can be already fulfilled by the definition request
Fixes https://github.com/clangd/clangd/issues/630
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D93763
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/test/xrefs.test
clang-tools-extra/clangd/tool/ClangdMain.cpp
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -339,6 +339,15 @@
Hidden,
};
+opt IncludesAsLinks{
+"includes-as-links",
+cat(Features),
+desc("Provide a document link to the files included with #include "
+ "directive. If set to false, include files can still be opened with "
+ "go to definition feature"),
+init(true)
+};
+
opt WorkerThreadsCount{
"j",
cat(Misc),
@@ -825,6 +834,7 @@
Opts.PreserveRecoveryASTType = RecoveryASTType;
Opts.FoldingRanges = FoldingRanges;
Opts.MemoryCleanup = getMemoryCleanupFunction();
+ Opts.IncludesAsLinks = IncludesAsLinks;
Opts.CodeComplete.IncludeIneligibleResults = IncludeIneligibleResults;
Opts.CodeComplete.Limit = LimitResults;
Index: clang-tools-extra/clangd/test/xrefs.test
===
--- clang-tools-extra/clangd/test/xrefs.test
+++ clang-tools-extra/clangd/test/xrefs.test
@@ -1,9 +1,9 @@
# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
---
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"extern int x;\nint x = 0;\nint y = x;"}}}
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"#include \nextern int x;\nint x = 0;\nint y = x;"}}}
---
-{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":2,"character":8}}}
+{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":3,"character":8}}}
# CHECK: "id": 1,
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT: "result": [
@@ -11,11 +11,11 @@
# CHECK-NEXT: "range": {
# CHECK-NEXT:"end": {
# CHECK-NEXT: "character": 5,
-# CHECK-NEXT: "line": 1
+# CHECK-NEXT: "line": 2
# CHECK-NEXT:},
# CHECK-NEXT:"start": {
# CHECK-NEXT: "character": 4,
-# CHECK-NEXT: "line": 1
+# CHECK-NEXT: "line": 2
# CHECK-NEXT:}
# CHECK-NEXT: },
# CHECK-NEXT: "uri": "file://{{.*}}/{{([A-Z]:/)?}}main.cpp"
@@ -23,7 +23,7 @@
# CHECK-NEXT: ]
---
# Toggle: we're on the definition, so jump to the declaration.
-{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":1,"character":4}}}
+{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":2,"character":4}}}
# CHECK: "id": 1,
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT: "result": [
@@ -31,18 +31,18 @@
# CHECK-NEXT: "range": {
# CHECK-NEXT:"end": {
# CHECK-NEXT: "character": 12,
-# CHECK-NEXT: "line": 0
+# CHECK-NEXT: "line": 1
# CHECK-NEXT:},
# CHECK-NEXT:"start": {
# CHECK-NEXT: "character": 11,
-# CHECK-NEXT: "line": 0
+# CHECK-NEXT: "line": 1
# CHECK-NEXT:}
# CHECK-NEXT: },
# CHECK-NEXT: "uri": "file://{{.*}}/{{([A-Z]:/)?}}main.cpp"
# CHECK-NEXT:}
# CHECK-NEXT: ]
---
-{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":2,"character":8}}}
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":3,"character":8}}}
# CHECK: "id": 1
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT: "result": [
@@ -51,11 +51,11 @@
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
# CHECK-NEXT: "character": 12,
-# CHECK-NEXT: "line": 0
+# CHECK-NEXT: "line": 1
# CHECK-NEXT: },
[PATCH] D93763: [clangd] Add a flag to disable the documentLinks LSP request
rapgenic updated this revision to Diff 313667.
rapgenic added a comment.
Fix formatting
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93763/new/
https://reviews.llvm.org/D93763
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/test/xrefs.test
clang-tools-extra/clangd/tool/ClangdMain.cpp
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -339,6 +339,15 @@
Hidden,
};
+opt IncludesAsLinks{
+"includes-as-links",
+cat(Features),
+desc("Provide a document link to the files included with #include "
+ "directive. If set to false, include files can still be opened with "
+ "go to definition feature"),
+init(true),
+};
+
opt WorkerThreadsCount{
"j",
cat(Misc),
@@ -825,6 +834,7 @@
Opts.PreserveRecoveryASTType = RecoveryASTType;
Opts.FoldingRanges = FoldingRanges;
Opts.MemoryCleanup = getMemoryCleanupFunction();
+ Opts.IncludesAsLinks = IncludesAsLinks;
Opts.CodeComplete.IncludeIneligibleResults = IncludeIneligibleResults;
Opts.CodeComplete.Limit = LimitResults;
Index: clang-tools-extra/clangd/test/xrefs.test
===
--- clang-tools-extra/clangd/test/xrefs.test
+++ clang-tools-extra/clangd/test/xrefs.test
@@ -1,9 +1,9 @@
# RUN: clangd -lit-test < %s | FileCheck -strict-whitespace %s
{"jsonrpc":"2.0","id":0,"method":"initialize","params":{"processId":123,"rootPath":"clangd","capabilities":{},"trace":"off"}}
---
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"extern int x;\nint x = 0;\nint y = x;"}}}
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"#include \nextern int x;\nint x = 0;\nint y = x;"}}}
---
-{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":2,"character":8}}}
+{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":3,"character":8}}}
# CHECK: "id": 1,
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT: "result": [
@@ -11,11 +11,11 @@
# CHECK-NEXT: "range": {
# CHECK-NEXT:"end": {
# CHECK-NEXT: "character": 5,
-# CHECK-NEXT: "line": 1
+# CHECK-NEXT: "line": 2
# CHECK-NEXT:},
# CHECK-NEXT:"start": {
# CHECK-NEXT: "character": 4,
-# CHECK-NEXT: "line": 1
+# CHECK-NEXT: "line": 2
# CHECK-NEXT:}
# CHECK-NEXT: },
# CHECK-NEXT: "uri": "file://{{.*}}/{{([A-Z]:/)?}}main.cpp"
@@ -23,7 +23,7 @@
# CHECK-NEXT: ]
---
# Toggle: we're on the definition, so jump to the declaration.
-{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":1,"character":4}}}
+{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":2,"character":4}}}
# CHECK: "id": 1,
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT: "result": [
@@ -31,18 +31,18 @@
# CHECK-NEXT: "range": {
# CHECK-NEXT:"end": {
# CHECK-NEXT: "character": 12,
-# CHECK-NEXT: "line": 0
+# CHECK-NEXT: "line": 1
# CHECK-NEXT:},
# CHECK-NEXT:"start": {
# CHECK-NEXT: "character": 11,
-# CHECK-NEXT: "line": 0
+# CHECK-NEXT: "line": 1
# CHECK-NEXT:}
# CHECK-NEXT: },
# CHECK-NEXT: "uri": "file://{{.*}}/{{([A-Z]:/)?}}main.cpp"
# CHECK-NEXT:}
# CHECK-NEXT: ]
---
-{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":2,"character":8}}}
+{"jsonrpc":"2.0","id":1,"method":"textDocument/documentHighlight","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":3,"character":8}}}
# CHECK: "id": 1
# CHECK-NEXT: "jsonrpc": "2.0",
# CHECK-NEXT: "result": [
@@ -51,11 +51,11 @@
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
# CHECK-NEXT: "character": 12,
-# CHECK-NEXT: "line": 0
+# CHECK-NEXT: "line": 1
# CHECK-NEXT: },
# CHECK-NEXT: "start": {
# CHECK-NEXT: "character": 11,
-# CHECK-NEXT: "line": 0
+# CHECK-NEXT: "line": 1
# CHECK-NEXT: }
# CHECK-NEXT: }
# CHECK-NEXT: },
@@ -64,11 +64,11 @@
# CHECK-NEXT: "range": {
# CHECK-NEXT: "end": {
# CHECK-NEXT: "character": 5,
-# CHECK-NEXT: "line": 1
+# CHECK-NEXT: "line": 2
# CHECK-NEXT: },
# CHECK-NEXT: "sta
[PATCH] D93763: [clangd] Add a flag to disable the documentLinks LSP request
rapgenic marked 4 inline comments as done.
rapgenic added a comment.
> In the future it may make sense to have other documentLinks too. (e.g.
> imagine a comment Configures the Frobnicator - we should linkify Frobnicator
> to point to a class, once documentLink supports target ranges). So we should
> decide if this feature is "disable documentLink" or "don't include included
> headers in documentLink". This affects e.g. naming of the flag, which is
> awkward to change later.
I think it's already like that, in that specific case, and it uses
goToDefinition instead: if I ctrl+click on a variable/class/function name in a
comment i can actually go to the definition, if I am not wrong.
As for the naming of the option, maybe the title of the patch is not too
accurate, however from the option description it's quite clear that it's only
related to include files, so any additional feature using documentLinks should
be easy to add while keeping this option too.
> Do we really want to add command-line flags for this or does it rather belong
> as a config option? (In which case we'd want to change the behavior of the
> method, not turn the method itself on/off, to allow the config option to vary
> across files in the usual way).
In my opinion (and also by looking at the already implemented options in
.clangd config file) the config file seems to be related to project specific
configurations (compilation flags, indexing, etc.), whereas the option that
this patch implements is more related to the "editing experience", which is
something one should not change very often. So I think that it might not be
worth putting it in the config file.
Comment at: clang-tools-extra/clangd/test/xrefs.test:4
---
-{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"extern
int x;\nint x = 0;\nint y = x;"}}}
+{"jsonrpc":"2.0","method":"textDocument/didOpen","params":{"textDocument":{"uri":"test:///main.cpp","languageId":"cpp","version":1,"text":"#include
\nextern int x;\nint x = 0;\nint y = x;"}}}
---
sammccall wrote:
> Having tests depend on external headers is a bit of a pain downstream - it
> means they need to run in an environment where the headers are available.
>
> We do this by necessity in `document-link.test` - maybe we should move this
> test here?
> (Personally I'm comfortable with this functionality only being covered by
> unit-tests, as it is now though).
I'm moving the test, as it is related to document links it seems pertinent to
put it there, if you prefer we can leave it out of course
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93763/new/
https://reviews.llvm.org/D93763
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D93763: [clangd] Add a flag to disable the documentLinks LSP request
rapgenic updated this revision to Diff 314718.
rapgenic marked an inline comment as done.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D93763/new/
https://reviews.llvm.org/D93763
Files:
clang-tools-extra/clangd/ClangdLSPServer.cpp
clang-tools-extra/clangd/ClangdLSPServer.h
clang-tools-extra/clangd/test/document-link.test
clang-tools-extra/clangd/tool/ClangdMain.cpp
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -339,6 +339,15 @@
Hidden,
};
+opt IncludesAsLinks{
+"includes-as-links",
+cat(Features),
+desc("Provide a document link to the files included with #include "
+ "directive. If set to false, include files can still be opened with "
+ "go to definition feature"),
+init(true),
+};
+
opt WorkerThreadsCount{
"j",
cat(Misc),
@@ -825,6 +834,7 @@
Opts.PreserveRecoveryASTType = RecoveryASTType;
Opts.FoldingRanges = FoldingRanges;
Opts.MemoryCleanup = getMemoryCleanupFunction();
+ Opts.IncludesAsLinks = IncludesAsLinks;
Opts.CodeComplete.IncludeIneligibleResults = IncludeIneligibleResults;
Opts.CodeComplete.Limit = LimitResults;
Index: clang-tools-extra/clangd/test/document-link.test
===
--- clang-tools-extra/clangd/test/document-link.test
+++ clang-tools-extra/clangd/test/document-link.test
@@ -37,6 +37,26 @@
# CHECK-NEXT:}
# CHECK-NEXT: ]
# CHECK-NEXT:}
+---
+# Go to definition on include (necessary when documentLinks are disabled)
+{"jsonrpc":"2.0","id":1,"method":"textDocument/definition","params":{"textDocument":{"uri":"test:///main.cpp"},"position":{"line":0,"character":15}}}
+# CHECK: "id": 1,
+# CHECK-NEXT: "jsonrpc": "2.0",
+# CHECK-NEXT: "result": [
+# CHECK-NEXT:{
+# CHECK-NEXT: "range": {
+# CHECK-NEXT:"end": {
+# CHECK-NEXT: "character": 0,
+# CHECK-NEXT: "line": 0
+# CHECK-NEXT:},
+# CHECK-NEXT:"start": {
+# CHECK-NEXT: "character": 0,
+# CHECK-NEXT: "line": 0
+# CHECK-NEXT:}
+# CHECK-NEXT: },
+# CHECK-NEXT: "uri": "file://{{.*}}/{{([A-Z]:/)?}}stdint.h"
+# CHECK-NEXT:}
+# CHECK-NEXT: ]
---
{"jsonrpc":"2.0","id":3,"method":"shutdown"}
Index: clang-tools-extra/clangd/ClangdLSPServer.h
===
--- clang-tools-extra/clangd/ClangdLSPServer.h
+++ clang-tools-extra/clangd/ClangdLSPServer.h
@@ -61,6 +61,10 @@
std::function TweakFilter = [](const Tweak &T) {
return !T.hidden(); // only enable non-hidden tweaks.
};
+/// If set, include files in #include directives are exposed to the clients
+/// as documentLinks. If disabled include files can still be opened with the
+/// go to definition feature.
+bool IncludesAsLinks = true;
};
ClangdLSPServer(Transport &Transp, const ThreadsafeFS &TFS,
Index: clang-tools-extra/clangd/ClangdLSPServer.cpp
===
--- clang-tools-extra/clangd/ClangdLSPServer.cpp
+++ clang-tools-extra/clangd/ClangdLSPServer.cpp
@@ -610,10 +610,6 @@
{"definitionProvider", true},
{"implementationProvider", true},
{"documentHighlightProvider", true},
-{"documentLinkProvider",
- llvm::json::Object{
- {"resolveProvider", false},
- }},
{"hoverProvider", true},
{"renameProvider", std::move(RenameProvider)},
{"selectionRangeProvider", true},
@@ -640,6 +636,12 @@
llvm::json::Object{{"scopes", buildHighlightScopeLookupTable()}}});
if (Opts.FoldingRanges)
Result.getObject("capabilities")->insert({"foldingRangeProvider", true});
+ if (Opts.IncludesAsLinks)
+// Currently we only provide documentLink for #included headers.
+// If that's turned off, let clients avoid sending the request altogether.
+Result.getObject("capabilities")
+->insert({"documentLinkProvider",
+ llvm::json::Object{{"resolveProvider", false}}});
Reply(std::move(Result));
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
