[PATCH] D116167: [clangd] Adjust compile flags so they work when applied to other file(type)s.
Sam McCall via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 22 08:01:54 PST 2021
sammccall created this revision.
sammccall added a reviewer: kadircet.
Herald added subscribers: usaxena95, arphaman.
sammccall requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.
It's reasonable to want to use the command from one file to compile another.
In particular, the command from a translation unit to parse a related header:
{"file": "foo.h", "command": "clang foo.cpp"}
This is largely what InterpolatingCompilationDatabase tries to do.
To do this correctly can require nontrivial changes to the argv, because the
file extension affects semantics. e.g. here we must add "-x c++header".
When external tools compile commands for different files, we should apply the
same adjustments. This is better than telling people to "fix their tools":
- simple e.g. python scripts shouldn't have to interpret clang argv
- this is a good way to represent the intent "parse header X in the context of file Y", which can work even if X is not self-contained. clangd does not support this today, but some other tools do, and we may one day.
This issue is discussed in https://github.com/clangd/clangd/issues/519
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D116167
Files:
clang-tools-extra/clangd/CompileCommands.cpp
clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
Index: clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
+++ clang-tools-extra/clangd/unittests/CompileCommandsTests.cpp
@@ -53,6 +53,18 @@
"foo.cc"));
}
+TEST(CommandMangler, FilenameMismatch) {
+ auto Mangler = CommandMangler::forTests();
+ Mangler.ClangPath = testPath("clang");
+ // Our compile flags refer to foo.cc...
+ std::vector<std::string> Cmd = {"clang", "foo.cc"};
+ // but we're applying it to foo.h...
+ Mangler.adjust(Cmd, "foo.h");
+ // so transferCompileCommand should add -x c++-header to preserve semantics.
+ EXPECT_THAT(
+ Cmd, ElementsAre(testPath("clang"), "-x", "c++-header", "--", "foo.h"));
+}
+
TEST(CommandMangler, ResourceDir) {
auto Mangler = CommandMangler::forTests();
Mangler.ResourceDir = testPath("fake/resources");
Index: clang-tools-extra/clangd/CompileCommands.cpp
===================================================================
--- clang-tools-extra/clangd/CompileCommands.cpp
+++ clang-tools-extra/clangd/CompileCommands.cpp
@@ -241,16 +241,38 @@
if (ArchOptCount < 2)
IndicesToDrop.clear();
+ // In some cases people may try to reuse the command from another file, e.g.
+ // { File: "foo.h", CommandLine: "clang foo.cpp" }.
+ // We assume the intent is to parse foo.h the same way as foo.cpp, or as if
+ // it were being included from foo.cpp.
+ //
+ // We're going to rewrite the command to refer to foo.h, and this may change
+ // its semantics (e.g. by parsing the file as C). If we do this, we should
+ // use transferCompileCommand to adjust the argv.
+ // In practice only the extension of the file matters, so do this only when
+ // it differs.
+ llvm::StringRef FileExtension = llvm::sys::path::extension(File);
+ llvm::Optional<std::string> TransferFrom;
+ auto SawInput = [&](llvm::StringRef Input) {
+ if (llvm::sys::path::extension(Input) != FileExtension)
+ TransferFrom.emplace(Input);
+ };
+
// Strip all the inputs and `--`. We'll put the input for the requested file
// explicitly at the end of the flags. This ensures modifications done in the
// following steps apply in more cases (like setting -x, which only affects
// inputs that come after it).
- for (auto *Input : ArgList.filtered(driver::options::OPT_INPUT))
+ for (auto *Input : ArgList.filtered(driver::options::OPT_INPUT)) {
+ SawInput(Input->getValue(0));
IndicesToDrop.push_back(Input->getIndex());
+ }
// Anything after `--` is also treated as input, drop them as well.
if (auto *DashDash =
ArgList.getLastArgNoClaim(driver::options::OPT__DASH_DASH)) {
- Cmd.resize(DashDash->getIndex() + 1); // +1 to account for Cmd[0].
+ auto DashDashIndex = DashDash->getIndex() + 1; // +1 accounts for Cmd[0]
+ for (unsigned I = DashDashIndex; I < Cmd.size(); ++I)
+ SawInput(Cmd[I]);
+ Cmd.resize(DashDashIndex);
}
llvm::sort(IndicesToDrop);
llvm::for_each(llvm::reverse(IndicesToDrop),
@@ -262,6 +284,24 @@
Cmd.push_back("--");
Cmd.push_back(File.str());
+ if (TransferFrom) {
+ tooling::CompileCommand TransferCmd;
+ TransferCmd.Filename = std::move(*TransferFrom);
+ TransferCmd.CommandLine = std::move(Cmd);
+ TransferCmd = transferCompileCommand(std::move(TransferCmd), File);
+ Cmd = std::move(TransferCmd.CommandLine);
+
+ // Restore the canonical "driver --opts -- filename" form we expect.
+ // FIXME: This is ugly and coupled. Make transferCompileCommand ensure it?
+ assert(!Cmd.empty() && Cmd.back() == File);
+ Cmd.pop_back();
+ if (!Cmd.empty() && Cmd.back() == "--")
+ Cmd.pop_back();
+ assert(!llvm::is_contained(Cmd, "--"));
+ Cmd.push_back("--");
+ Cmd.push_back(File.str());
+ }
+
for (auto &Edit : Config::current().CompileFlags.Edits)
Edit(Cmd);
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D116167.395871.patch
Type: text/x-patch
Size: 3946 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20211222/98d081b5/attachment.bin>
More information about the cfe-commits
mailing list