[PATCH] D51729: [Tooling] JSONCompilationDatabasePlugin infers compile commands for missing files
Lekensteyn added a comment. In https://reviews.llvm.org/D51729#1288360, @sammccall wrote: > > I'm not entirely sure what to do here. The old behavior works great in > > cases where a complete database is available (produced by CMake). The new > > behavior might work better for clangd (?), but it breaks a use case (see > > above). > > For clangd, but also clang-tidy and clang-query when the user *does* want to > use it on files not represented in the CDB. (e.g. stale or headers) > There's indeed a tension here, because the CDB discovery needs to have a > default configuration. Support for querying header files seems valuable and a good argument to keep this change. > That said, in this case the behavior looks appropriate to me: you've > explicitly specified files on the command line, ignoring them and returning > with status 0 seems surprising. The test explicitly lists them, but in practice the `find` or `grep -rl` provides the file names which might contain uninteresting matches that are not present in the CDB. As a compromise, it could return with status 1 but proceed querying files that were successfully parsed. > For the case of "search over all TUs in CDB", the CDB does offer the ability > to list TUs and iterate over compile commands, and ClangTool lets you run in > this mode. We've discussed in the past adding a filename filter to > `AllTUsExecutor`, which would be useful for this purpose and others. @ioeric `AllTUsToolExecutor` looks useful to enable concurrency, but a filename filter would not help in my case. The reason I do a "cheap" grep first before using clang-query is because building the AST is slow and consumes a lot of memory (after ~1k .c files, 12G was in use). Querying the CDB and filtering out entries is currently possible, but quite verbose: grep -le 'search term' $(jq -r '.[].file' compile_commands.json) | xargs clang-query -c 'm ...' Thank you for the feedback, my concerns with this CDB patch has been resolved. Repository: rC Clang https://reviews.llvm.org/D51729 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51729: [Tooling] JSONCompilationDatabasePlugin infers compile commands for missing files
sammccall added a comment.
In https://reviews.llvm.org/D51729#1287421, @Lekensteyn wrote:
> Before this patch, missing compilation database entries resulted in "Skipping
> Compile command not found." which is assumed by the tests in this
> clang-query patch: https://reviews.llvm.org/D54109
>
> After this patch, a command is inferred, but this is not always desired.
> Use case: find all function calls with a certain name using the compilation
> database produced by CMake.
> Example command:
>
> clang-query -p=/tmp/wsbuild -c 'set output print' -c 'm
> callExpr(callee(functionDecl(hasName("uat_new"' $(grep -rl uat_new
> epan/dissectors/)
>
>
> Expected result for some template files which do not exist in the compilation
> database:
>
> Skipping .../epan/dissectors/asn1/x509af/packet-x509af-template.c. Compile
> command not found.
>
>
> Actual result (clang-query tries to parse the contents which will fail since
> it is not an actual C source file):
>
> .../epan/dissectors/asn1/x509af/packet-x509af-template.c:18:10: fatal
> error: 'packet-ber.h' file not found
>
>
> I'm not entirely sure what to do here. The old behavior works great in cases
> where a complete database is available (produced by CMake). The new behavior
> might work better for clangd (?), but it breaks a use case (see above).
For clangd, but also clang-tidy and clang-query when the user *does* want to
use it on files not represented in the CDB. (e.g. stale or headers)
There's indeed a tension here, because the CDB discovery needs to have a
default configuration.
That said, in this case the behavior looks appropriate to me: you've explicitly
specified files on the command line, ignoring them and returning with status 0
seems surprising.
For the case of "search over all TUs in CDB", the CDB does offer the ability to
list TUs and iterate over compile commands, and ClangTool lets you run in this
mode. We've discussed in the past adding a filename filter to `AllTUsExecutor`,
which would be useful for this purpose and others. @ioeric
Repository:
rC Clang
https://reviews.llvm.org/D51729
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51729: [Tooling] JSONCompilationDatabasePlugin infers compile commands for missing files
Lekensteyn added a comment.
Before this patch, missing compilation database entries resulted in "Skipping
Compile command not found." which is assumed by the tests in this
clang-query patch: https://reviews.llvm.org/D54109
After this patch, a command is inferred, but this is not always desired.
Use case: find all function calls with a certain name using the compilation
database produced by CMake.
Example command:
clang-query -p=/tmp/wsbuild -c 'set output print' -c 'm
callExpr(callee(functionDecl(hasName("uat_new"' $(grep -rl uat_new
epan/dissectors/)
Expected result for some template files which do not exist in the compilation
database:
Skipping .../epan/dissectors/asn1/x509af/packet-x509af-template.c. Compile
command not found.
Actual result (clang-query tries to parse the contents which will fail since it
is not an actual C source file):
.../epan/dissectors/asn1/x509af/packet-x509af-template.c:18:10: fatal error:
'packet-ber.h' file not found
I'm not entirely sure what to do here. The old behavior works great in cases
where a complete database is available (produced by CMake). The new behavior
might work better for clangd (?), but it breaks a use case (see above).
Repository:
rC Clang
https://reviews.llvm.org/D51729
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D51729: [Tooling] JSONCompilationDatabasePlugin infers compile commands for missing files
Sorry about that. I wasn't familiar with the python bindings. Your bisect is correct, we changed the behavior that test is testing: now an approximate match will be returned. I guess either: - just remove that test (depending on how we feel about smoke-testing vs exhaustively testing wrapperl like this) - change it to verify exactly what the fuzzy-matched result is, e.g. - add -DFEATURE=0 to the first compile command for "Project.cpp" in the test CDB so it's easy to recognize - make the lookup key "/home/john.doe/MyProject/Project.h" so it's obvious what the match should be - verify that the lookup result has "-DFEATURE=0" and "Project.h" in its args It seems to me that test_lookup_succeed should also verify what gets returned (otherwise it's not really testing anything at this point). I can try to put together a patch, though I don't currently have a setup to test it - let me know if this would be useful! On Fri, Oct 12, 2018 at 8:38 AM Michał Górny via Phabricator < revi...@reviews.llvm.org> wrote: > mgorny added a comment. > > Unless my bisect is mistaken, this broke Clang Python binding test: > > https://github.com/llvm-mirror/clang/blob/master/bindings/python/tests/cindex/test_cdb.py#L34 > > The test apparently assumes that compilation database will not return > anything for a file that does not exist. Could you please suggest how the > test should be adjusted for this change? > > > Repository: > rC Clang > > https://reviews.llvm.org/D51729 > > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51729: [Tooling] JSONCompilationDatabasePlugin infers compile commands for missing files
mgorny added a comment. Unless my bisect is mistaken, this broke Clang Python binding test: https://github.com/llvm-mirror/clang/blob/master/bindings/python/tests/cindex/test_cdb.py#L34 The test apparently assumes that compilation database will not return anything for a file that does not exist. Could you please suggest how the test should be adjusted for this change? Repository: rC Clang https://reviews.llvm.org/D51729 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51729: [Tooling] JSONCompilationDatabasePlugin infers compile commands for missing files
This revision was automatically updated to reflect the committed changes.
Closed by commit rC342228: [Tooling] JSONCompilationDatabasePlugin infers
compile commands for missing… (authored by sammccall, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D51729?vs=164192=165472#toc
Repository:
rC Clang
https://reviews.llvm.org/D51729
Files:
lib/Tooling/JSONCompilationDatabase.cpp
test/Tooling/auto-detect-from-source.cpp
Index: test/Tooling/auto-detect-from-source.cpp
===
--- test/Tooling/auto-detect-from-source.cpp
+++ test/Tooling/auto-detect-from-source.cpp
@@ -1,8 +1,12 @@
// RUN: rm -rf %t
// RUN: mkdir %t
-// RUN: echo "[{\"directory\":\".\",\"command\":\"clang++ -c
%/t/test.cpp\",\"file\":\"%/t/test.cpp\"}]" | sed -e 's/\\//g' >
%t/compile_commands.json
+// RUN: echo "[{\"directory\":\".\",\"command\":\"clang++ -DSECRET=XYZZY -c
%/t/test.cpp\",\"file\":\"%/t/test.cpp\"}]" | sed -e 's/\\//g' >
%t/compile_commands.json
// RUN: cp "%s" "%t/test.cpp"
// RUN: not clang-check "%t/test.cpp" 2>&1 | FileCheck %s
-// CHECK: C++ requires
-invalid;
+// CHECK: XYZZY
+SECRET;
+
+// Copy to a different file, and rely on the command being inferred.
+// RUN: cp "%s" "%t/other.cpp"
+// RUN: not clang-check "%t/other.cpp" 2>&1 | FileCheck %s
Index: lib/Tooling/JSONCompilationDatabase.cpp
===
--- lib/Tooling/JSONCompilationDatabase.cpp
+++ lib/Tooling/JSONCompilationDatabase.cpp
@@ -157,13 +157,16 @@
return parser.parse();
}
+// This plugin locates a nearby compile_command.json file, and also infers
+// compile commands for files not present in the database.
class JSONCompilationDatabasePlugin : public CompilationDatabasePlugin {
std::unique_ptr
loadFromDirectory(StringRef Directory, std::string ) override {
SmallString<1024> JSONDatabasePath(Directory);
llvm::sys::path::append(JSONDatabasePath, "compile_commands.json");
-return JSONCompilationDatabase::loadFromFile(
+auto Base = JSONCompilationDatabase::loadFromFile(
JSONDatabasePath, ErrorMessage, JSONCommandLineSyntax::AutoDetect);
+return Base ? inferMissingCompileCommands(std::move(Base)) : nullptr;
}
};
Index: test/Tooling/auto-detect-from-source.cpp
===
--- test/Tooling/auto-detect-from-source.cpp
+++ test/Tooling/auto-detect-from-source.cpp
@@ -1,8 +1,12 @@
// RUN: rm -rf %t
// RUN: mkdir %t
-// RUN: echo "[{\"directory\":\".\",\"command\":\"clang++ -c %/t/test.cpp\",\"file\":\"%/t/test.cpp\"}]" | sed -e 's/\\//g' > %t/compile_commands.json
+// RUN: echo "[{\"directory\":\".\",\"command\":\"clang++ -DSECRET=XYZZY -c %/t/test.cpp\",\"file\":\"%/t/test.cpp\"}]" | sed -e 's/\\//g' > %t/compile_commands.json
// RUN: cp "%s" "%t/test.cpp"
// RUN: not clang-check "%t/test.cpp" 2>&1 | FileCheck %s
-// CHECK: C++ requires
-invalid;
+// CHECK: XYZZY
+SECRET;
+
+// Copy to a different file, and rely on the command being inferred.
+// RUN: cp "%s" "%t/other.cpp"
+// RUN: not clang-check "%t/other.cpp" 2>&1 | FileCheck %s
Index: lib/Tooling/JSONCompilationDatabase.cpp
===
--- lib/Tooling/JSONCompilationDatabase.cpp
+++ lib/Tooling/JSONCompilationDatabase.cpp
@@ -157,13 +157,16 @@
return parser.parse();
}
+// This plugin locates a nearby compile_command.json file, and also infers
+// compile commands for files not present in the database.
class JSONCompilationDatabasePlugin : public CompilationDatabasePlugin {
std::unique_ptr
loadFromDirectory(StringRef Directory, std::string ) override {
SmallString<1024> JSONDatabasePath(Directory);
llvm::sys::path::append(JSONDatabasePath, "compile_commands.json");
-return JSONCompilationDatabase::loadFromFile(
+auto Base = JSONCompilationDatabase::loadFromFile(
JSONDatabasePath, ErrorMessage, JSONCommandLineSyntax::AutoDetect);
+return Base ? inferMissingCompileCommands(std::move(Base)) : nullptr;
}
};
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51729: [Tooling] JSONCompilationDatabasePlugin infers compile commands for missing files
bkramer accepted this revision. bkramer added a comment. This revision is now accepted and ready to land. I like it Repository: rC Clang https://reviews.llvm.org/D51729 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51729: [Tooling] JSONCompilationDatabasePlugin infers compile commands for missing files
sammccall created this revision.
sammccall added a reviewer: bkramer.
Herald added subscribers: cfe-commits, kadircet, ioeric, ilya-biryukov.
See the existing InterpolatingCompilationDatabase for details on how this works.
We've been using this in clangd for a while, the heuristics seem to work well.
Repository:
rC Clang
https://reviews.llvm.org/D51729
Files:
lib/Tooling/JSONCompilationDatabase.cpp
test/Tooling/auto-detect-from-source.cpp
Index: test/Tooling/auto-detect-from-source.cpp
===
--- test/Tooling/auto-detect-from-source.cpp
+++ test/Tooling/auto-detect-from-source.cpp
@@ -1,8 +1,12 @@
// RUN: rm -rf %t
// RUN: mkdir %t
-// RUN: echo "[{\"directory\":\".\",\"command\":\"clang++ -c
%/t/test.cpp\",\"file\":\"%/t/test.cpp\"}]" | sed -e 's/\\//g' >
%t/compile_commands.json
+// RUN: echo "[{\"directory\":\".\",\"command\":\"clang++ -DSECRET=XYZZY -c
%/t/test.cpp\",\"file\":\"%/t/test.cpp\"}]" | sed -e 's/\\//g' >
%t/compile_commands.json
// RUN: cp "%s" "%t/test.cpp"
// RUN: not clang-check "%t/test.cpp" 2>&1 | FileCheck %s
-// CHECK: C++ requires
-invalid;
+// CHECK: XYZZY
+SECRET;
+
+// Copy to a different file, and rely on the command being inferred.
+// RUN: cp "%s" "%t/other.cpp"
+// RUN: not clang-check "%t/other.cpp" 2>&1 | FileCheck %s
Index: lib/Tooling/JSONCompilationDatabase.cpp
===
--- lib/Tooling/JSONCompilationDatabase.cpp
+++ lib/Tooling/JSONCompilationDatabase.cpp
@@ -157,13 +157,16 @@
return parser.parse();
}
+// This plugin locates a nearby compile_command.json file, and also infers
+// compile commands for files not present in the database.
class JSONCompilationDatabasePlugin : public CompilationDatabasePlugin {
std::unique_ptr
loadFromDirectory(StringRef Directory, std::string ) override {
SmallString<1024> JSONDatabasePath(Directory);
llvm::sys::path::append(JSONDatabasePath, "compile_commands.json");
-return JSONCompilationDatabase::loadFromFile(
+auto Base = JSONCompilationDatabase::loadFromFile(
JSONDatabasePath, ErrorMessage, JSONCommandLineSyntax::AutoDetect);
+return Base ? inferMissingCompileCommands(std::move(Base)) : nullptr;
}
};
Index: test/Tooling/auto-detect-from-source.cpp
===
--- test/Tooling/auto-detect-from-source.cpp
+++ test/Tooling/auto-detect-from-source.cpp
@@ -1,8 +1,12 @@
// RUN: rm -rf %t
// RUN: mkdir %t
-// RUN: echo "[{\"directory\":\".\",\"command\":\"clang++ -c %/t/test.cpp\",\"file\":\"%/t/test.cpp\"}]" | sed -e 's/\\//g' > %t/compile_commands.json
+// RUN: echo "[{\"directory\":\".\",\"command\":\"clang++ -DSECRET=XYZZY -c %/t/test.cpp\",\"file\":\"%/t/test.cpp\"}]" | sed -e 's/\\//g' > %t/compile_commands.json
// RUN: cp "%s" "%t/test.cpp"
// RUN: not clang-check "%t/test.cpp" 2>&1 | FileCheck %s
-// CHECK: C++ requires
-invalid;
+// CHECK: XYZZY
+SECRET;
+
+// Copy to a different file, and rely on the command being inferred.
+// RUN: cp "%s" "%t/other.cpp"
+// RUN: not clang-check "%t/other.cpp" 2>&1 | FileCheck %s
Index: lib/Tooling/JSONCompilationDatabase.cpp
===
--- lib/Tooling/JSONCompilationDatabase.cpp
+++ lib/Tooling/JSONCompilationDatabase.cpp
@@ -157,13 +157,16 @@
return parser.parse();
}
+// This plugin locates a nearby compile_command.json file, and also infers
+// compile commands for files not present in the database.
class JSONCompilationDatabasePlugin : public CompilationDatabasePlugin {
std::unique_ptr
loadFromDirectory(StringRef Directory, std::string ) override {
SmallString<1024> JSONDatabasePath(Directory);
llvm::sys::path::append(JSONDatabasePath, "compile_commands.json");
-return JSONCompilationDatabase::loadFromFile(
+auto Base = JSONCompilationDatabase::loadFromFile(
JSONDatabasePath, ErrorMessage, JSONCommandLineSyntax::AutoDetect);
+return Base ? inferMissingCompileCommands(std::move(Base)) : nullptr;
}
};
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
