[clang-tools-extra] [clang-include-cleaner] Fix incorrect directory issue for writing files (PR #111375)
Byoungchan Lee via cfe-commits
cfe-commits at lists.llvm.org
Sun Oct 13 04:24:53 PDT 2024
https://github.com/bc-lee updated https://github.com/llvm/llvm-project/pull/111375
>From 23b90bba12c010e5882e09e9f6b765a7281324aa Mon Sep 17 00:00:00 2001
From: Byoungchan Lee <byoungchan.lee at gmx.com>
Date: Mon, 7 Oct 2024 22:19:38 +0900
Subject: [PATCH 1/8] [clang-include-cleaner] Fix incorrect directory issue for
writing files
If the current working directory of `clang-include-cleaner` differs from
the directory of the input files specified in the compilation database,
it doesn't adjust the input file paths properly. As a result,
`clang-include-cleaner` either writes files to the wrong directory or
fails to write files altogether.
This pull request fixes the issue by checking whether the input file
path is absolute. If it isn't, the path is concatenated with
the directory of the input file specified in the compilation database.
Fixes #110843.
---
.../include-cleaner/tool/IncludeCleaner.cpp | 24 +++++++++++++++++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
index 080099adc9a07a..1cb8b9c4ae99b7 100644
--- a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
+++ b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
@@ -173,11 +173,22 @@ class Action : public clang::ASTFrontendAction {
if (!HTMLReportPath.empty())
writeHTML();
+ // Source File's path relative to compilation database.
llvm::StringRef Path =
SM.getFileEntryRefForID(SM.getMainFileID())->getName();
assert(!Path.empty() && "Main file path not known?");
llvm::StringRef Code = SM.getBufferData(SM.getMainFileID());
+ auto Cwd = getCompilerInstance()
+ .getFileManager()
+ .getVirtualFileSystem()
+ .getCurrentWorkingDirectory();
+ if (Cwd.getError()) {
+ llvm::errs() << "Failed to get current working directory: "
+ << Cwd.getError().message() << "\n";
+ return;
+ }
+
auto Results =
analyze(AST.Roots, PP.MacroReferences, PP.Includes, &PI,
getCompilerInstance().getPreprocessor(), HeaderFilter);
@@ -201,8 +212,17 @@ class Action : public clang::ASTFrontendAction {
}
}
- if (!Results.Missing.empty() || !Results.Unused.empty())
- EditedFiles.try_emplace(Path, Final);
+ if (!Results.Missing.empty() || !Results.Unused.empty()) {
+ auto Sept = llvm::sys::path::get_separator();
+ // Adjust the path to be absolute if it is not.
+ std::string FullPath;
+ if (llvm::sys::path::is_absolute(Path))
+ FullPath = Path.str();
+ else
+ FullPath = Cwd.get() + Sept.str() + Path.str();
+
+ EditedFiles.try_emplace(FullPath, Final);
+ }
}
void writeHTML() {
>From eef397cf22e062cf024ffcd84fef7bdf98833351 Mon Sep 17 00:00:00 2001
From: Byoungchan Lee <byoungchan.lee at gmx.com>
Date: Wed, 9 Oct 2024 19:43:02 +0900
Subject: [PATCH 2/8] Fix path issue identified by @kadircet
Some Bazel systems might have a read-only file system in the build
directory. In such cases, the output file should be written to
the original source file. Adjusted the logic to find the correct
output directory based on the compilation database and
the current working directory.
---
.../include-cleaner/tool/IncludeCleaner.cpp | 95 ++++++++++++++-----
1 file changed, 73 insertions(+), 22 deletions(-)
diff --git a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
index 1cb8b9c4ae99b7..4481601140e3fe 100644
--- a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
+++ b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
@@ -179,16 +179,6 @@ class Action : public clang::ASTFrontendAction {
assert(!Path.empty() && "Main file path not known?");
llvm::StringRef Code = SM.getBufferData(SM.getMainFileID());
- auto Cwd = getCompilerInstance()
- .getFileManager()
- .getVirtualFileSystem()
- .getCurrentWorkingDirectory();
- if (Cwd.getError()) {
- llvm::errs() << "Failed to get current working directory: "
- << Cwd.getError().message() << "\n";
- return;
- }
-
auto Results =
analyze(AST.Roots, PP.MacroReferences, PP.Includes, &PI,
getCompilerInstance().getPreprocessor(), HeaderFilter);
@@ -212,17 +202,8 @@ class Action : public clang::ASTFrontendAction {
}
}
- if (!Results.Missing.empty() || !Results.Unused.empty()) {
- auto Sept = llvm::sys::path::get_separator();
- // Adjust the path to be absolute if it is not.
- std::string FullPath;
- if (llvm::sys::path::is_absolute(Path))
- FullPath = Path.str();
- else
- FullPath = Cwd.get() + Sept.str() + Path.str();
-
- EditedFiles.try_emplace(FullPath, Final);
- }
+ if (!Results.Missing.empty() || !Results.Unused.empty())
+ EditedFiles.try_emplace(Path, Final);
}
void writeHTML() {
@@ -300,6 +281,42 @@ std::function<bool(llvm::StringRef)> headerFilter() {
};
}
+llvm::ErrorOr<std::string> getCurrentWorkingDirectory() {
+ llvm::SmallString<256> CurrentPath;
+ if (const std::error_code EC = llvm::sys::fs::current_path(CurrentPath))
+ return llvm::ErrorOr<std::string>(EC);
+ return std::string(CurrentPath.str());
+}
+
+std::optional<std::string>
+adjustCompilationPath(const std::string &Filename, const std::string &Directory,
+ const std::string &CurrentWorkingDirectory) {
+ if (llvm::sys::path::is_absolute(Filename)) {
+ // If the file path is already absolute, use it as is.
+ return Filename;
+ }
+
+ auto Sept = llvm::sys::path::get_separator().str();
+ // First, try to find the file based on the compilation database.
+ std::string FilePath = Directory + Sept + Filename;
+ // Check if it is writable.
+ if (llvm::sys::fs::access(FilePath, llvm::sys::fs::AccessMode::Write) !=
+ std::error_code()) {
+ // If not, try to find the file based on the current working
+ // directory, as some Bazel invocations may not set the compilation
+ // invocation's filesystem as non-writable. In such cases, we can
+ // find the file based on the current working directory.
+ FilePath =
+ static_cast<std::string>(CurrentWorkingDirectory) + Sept + Filename;
+ if (llvm::sys::fs::access(FilePath, llvm::sys::fs::AccessMode::Write) !=
+ std::error_code()) {
+ llvm::errs() << "Failed to find a writable path for " << Filename << "\n";
+ return std::nullopt;
+ }
+ }
+ return FilePath;
+}
+
} // namespace
} // namespace include_cleaner
} // namespace clang
@@ -325,7 +342,32 @@ int main(int argc, const char **argv) {
}
}
- clang::tooling::ClangTool Tool(OptionsParser->getCompilations(),
+ auto &CompilationDatabase = OptionsParser->getCompilations();
+ // Mapping of edited file paths to their actual paths.
+ std::map<std::string, std::string> EditedFilesToActualFiles;
+ if (Edit) {
+ std::vector<clang::tooling::CompileCommand> Commands =
+ CompilationDatabase.getAllCompileCommands();
+ auto CurrentWorkingDirectory = getCurrentWorkingDirectory();
+ // if (CurrentWorkingDirectory.get()
+ if (auto EC = CurrentWorkingDirectory.getError()) {
+ llvm::errs() << "Failed to get current working directory: "
+ << EC.message() << "\n";
+ return 1;
+ }
+
+ for (const auto &Command : Commands) {
+ auto AdjustedFilePath = adjustCompilationPath(
+ Command.Filename, Command.Directory, CurrentWorkingDirectory.get());
+ if (!AdjustedFilePath.has_value()) {
+ // We already printed an error message.
+ return 1;
+ }
+ EditedFilesToActualFiles[Command.Filename] = AdjustedFilePath.value();
+ }
+ }
+
+ clang::tooling::ClangTool Tool(CompilationDatabase,
OptionsParser->getSourcePathList());
auto HeaderFilter = headerFilter();
@@ -336,6 +378,15 @@ int main(int argc, const char **argv) {
if (Edit) {
for (const auto &NameAndContent : Factory.editedFiles()) {
llvm::StringRef FileName = NameAndContent.first();
+ auto AdjustedFilePathIter = EditedFilesToActualFiles.find(FileName.str());
+ if (AdjustedFilePathIter != EditedFilesToActualFiles.end()) {
+ FileName = AdjustedFilePathIter->second;
+ } else {
+ llvm::errs() << "Failed to find the actual path for " << FileName
+ << "\n";
+ ++Errors;
+ }
+
const std::string &FinalCode = NameAndContent.second;
if (auto Err = llvm::writeToOutput(
FileName, [&](llvm::raw_ostream &OS) -> llvm::Error {
>From 2409d6f41be2933846b7ac21244cd59463c8e917 Mon Sep 17 00:00:00 2001
From: Byoungchan Lee <byoungchan.lee at gmx.com>
Date: Thu, 10 Oct 2024 18:02:27 +0900
Subject: [PATCH 3/8] Apply code review feedback
- Store the compilation database file as absolute path
- Instead of using `llvm::sys::fs::current_path`,
use `FileSystem::makeAbsolute` to get the absolute path
of various files
- Add more comments to explain the intent of the code
---
.../include-cleaner/tool/IncludeCleaner.cpp | 152 ++++++++++--------
1 file changed, 81 insertions(+), 71 deletions(-)
diff --git a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
index 4481601140e3fe..c0363b3d2bdb56 100644
--- a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
+++ b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
@@ -173,10 +173,10 @@ class Action : public clang::ASTFrontendAction {
if (!HTMLReportPath.empty())
writeHTML();
- // Source File's path relative to compilation database.
- llvm::StringRef Path =
- SM.getFileEntryRefForID(SM.getMainFileID())->getName();
- assert(!Path.empty() && "Main file path not known?");
+ // Source File's path of compiler invocation, converted to absolute path.
+ llvm::SmallString<256> AbsPath(SM.getFileEntryRefForID(SM.getMainFileID())->getName());
+ assert(!AbsPath.empty() && "Main file path not known?");
+ SM.getFileManager().makeAbsolutePath(AbsPath);
llvm::StringRef Code = SM.getBufferData(SM.getMainFileID());
auto Results =
@@ -186,7 +186,7 @@ class Action : public clang::ASTFrontendAction {
Results.Missing.clear();
if (!Remove)
Results.Unused.clear();
- std::string Final = fixIncludes(Results, Path, Code, getStyle(Path));
+ std::string Final = fixIncludes(Results, AbsPath, Code, getStyle(AbsPath));
if (Print.getNumOccurrences()) {
switch (Print) {
@@ -203,7 +203,7 @@ class Action : public clang::ASTFrontendAction {
}
if (!Results.Missing.empty() || !Results.Unused.empty())
- EditedFiles.try_emplace(Path, Final);
+ EditedFiles.try_emplace(AbsPath, Final);
}
void writeHTML() {
@@ -281,42 +281,6 @@ std::function<bool(llvm::StringRef)> headerFilter() {
};
}
-llvm::ErrorOr<std::string> getCurrentWorkingDirectory() {
- llvm::SmallString<256> CurrentPath;
- if (const std::error_code EC = llvm::sys::fs::current_path(CurrentPath))
- return llvm::ErrorOr<std::string>(EC);
- return std::string(CurrentPath.str());
-}
-
-std::optional<std::string>
-adjustCompilationPath(const std::string &Filename, const std::string &Directory,
- const std::string &CurrentWorkingDirectory) {
- if (llvm::sys::path::is_absolute(Filename)) {
- // If the file path is already absolute, use it as is.
- return Filename;
- }
-
- auto Sept = llvm::sys::path::get_separator().str();
- // First, try to find the file based on the compilation database.
- std::string FilePath = Directory + Sept + Filename;
- // Check if it is writable.
- if (llvm::sys::fs::access(FilePath, llvm::sys::fs::AccessMode::Write) !=
- std::error_code()) {
- // If not, try to find the file based on the current working
- // directory, as some Bazel invocations may not set the compilation
- // invocation's filesystem as non-writable. In such cases, we can
- // find the file based on the current working directory.
- FilePath =
- static_cast<std::string>(CurrentWorkingDirectory) + Sept + Filename;
- if (llvm::sys::fs::access(FilePath, llvm::sys::fs::AccessMode::Write) !=
- std::error_code()) {
- llvm::errs() << "Failed to find a writable path for " << Filename << "\n";
- return std::nullopt;
- }
- }
- return FilePath;
-}
-
} // namespace
} // namespace include_cleaner
} // namespace clang
@@ -342,33 +306,84 @@ int main(int argc, const char **argv) {
}
}
- auto &CompilationDatabase = OptionsParser->getCompilations();
- // Mapping of edited file paths to their actual paths.
- std::map<std::string, std::string> EditedFilesToActualFiles;
+ auto VFS = llvm::vfs::getRealFileSystem();
+ auto &CDB = OptionsParser->getCompilations();
+ // CDBToAbsPaths is a map from the path in the compilation database to the
+ // writable absolute path of the file.
+ std::map<std::string, std::string> CDBToAbsPaths;
if (Edit) {
- std::vector<clang::tooling::CompileCommand> Commands =
- CompilationDatabase.getAllCompileCommands();
- auto CurrentWorkingDirectory = getCurrentWorkingDirectory();
- // if (CurrentWorkingDirectory.get()
- if (auto EC = CurrentWorkingDirectory.getError()) {
- llvm::errs() << "Failed to get current working directory: "
- << EC.message() << "\n";
- return 1;
- }
-
- for (const auto &Command : Commands) {
- auto AdjustedFilePath = adjustCompilationPath(
- Command.Filename, Command.Directory, CurrentWorkingDirectory.get());
- if (!AdjustedFilePath.has_value()) {
- // We already printed an error message.
+ // if Edit is enabled, `Factory.editedFiles()` will contain the final code,
+ // along with the path given in the compilation database. That path can be
+ // absolute or relative, and if it is relative, it is relative to the
+ // "Directory" field in the compilation database. We need to make it
+ // absolute to write the final code to the correct path.
+ // There are several cases to consider:
+ // 1. The "Directory" field isn't same as the current working directory.
+ // 2. The file path resolved from the "Directory" field is not writable.
+ // For these cases, we need to find a writable path for the file.
+ // To effectively handle these cases, we only need to consider
+ // the files from `getSourcePathList()` that are present in the compilation
+ // database.
+ for (auto &Source : OptionsParser->getSourcePathList()) {
+ llvm::SmallString<256> AbsPath(Source);
+ if (auto Err = VFS->makeAbsolute(AbsPath)) {
+ llvm::errs() << "Failed to get absolute path for " << Source << " : "
+ << Err.message() << '\n';
return 1;
}
- EditedFilesToActualFiles[Command.Filename] = AdjustedFilePath.value();
+ std::vector<clang::tooling::CompileCommand> Cmds =
+ CDB.getCompileCommands(AbsPath);
+ if (Cmds.empty()) {
+ // Try with the original path.
+ Cmds = CDB.getCompileCommands(Source);
+ if (Cmds.empty()) {
+ continue;
+ }
+ }
+ // We only need the first command to get the directory.
+ auto Cmd = Cmds[0];
+ llvm::SmallString<256> CDBPath(Cmd.Filename);
+ std::string Directory(Cmd.Directory);
+
+ if (llvm::sys::path::is_absolute(CDBPath)) {
+ // If the path in the compilation database is already absolute, we don't
+ // need to do anything.
+ CDBToAbsPaths[static_cast<std::string>(CDBPath)] =
+ static_cast<std::string>(AbsPath);
+ } else {
+ auto Sept = llvm::sys::path::get_separator();
+ // First, try to find the file based on the compilation database.
+ llvm::Twine FilePathTwine = Directory + Sept + CDBPath;
+ llvm::SmallString<256> FilePath;
+ FilePathTwine.toVector(FilePath);
+ // Check if it is writable.
+ if (llvm::sys::fs::access(FilePath, llvm::sys::fs::AccessMode::Write) !=
+ std::error_code()) {
+ // If not, try to find the file based on the current working
+ // directory, as some Bazel invocations may not set the compilation
+ // invocation's filesystem as non-writable. In such cases, we can
+ // find the file based on the current working directory.
+ FilePath = Source;
+ if (auto EC = VFS->makeAbsolute(CDBPath)) {
+ llvm::errs() << "Failed to get absolute path for " << CDBPath
+ << " : " << EC.message() << '\n';
+ return 1;
+ }
+ if (llvm::sys::fs::access(FilePath,
+ llvm::sys::fs::AccessMode::Write) !=
+ std::error_code()) {
+ llvm::errs() << "Failed to find a writable path for " << Source
+ << '\n';
+ return 1;
+ }
+ }
+ CDBToAbsPaths[static_cast<std::string>(CDBPath)] =
+ static_cast<std::string>(FilePath);
+ }
}
}
- clang::tooling::ClangTool Tool(CompilationDatabase,
- OptionsParser->getSourcePathList());
+ clang::tooling::ClangTool Tool(CDB, OptionsParser->getSourcePathList());
auto HeaderFilter = headerFilter();
if (!HeaderFilter)
@@ -378,14 +393,9 @@ int main(int argc, const char **argv) {
if (Edit) {
for (const auto &NameAndContent : Factory.editedFiles()) {
llvm::StringRef FileName = NameAndContent.first();
- auto AdjustedFilePathIter = EditedFilesToActualFiles.find(FileName.str());
- if (AdjustedFilePathIter != EditedFilesToActualFiles.end()) {
- FileName = AdjustedFilePathIter->second;
- } else {
- llvm::errs() << "Failed to find the actual path for " << FileName
- << "\n";
- ++Errors;
- }
+ if (auto It = CDBToAbsPaths.find(FileName.str());
+ It != CDBToAbsPaths.end())
+ FileName = It->second;
const std::string &FinalCode = NameAndContent.second;
if (auto Err = llvm::writeToOutput(
>From 0c9477d3d7f2eb14f221a0a36cbd9dfd75015ef1 Mon Sep 17 00:00:00 2001
From: Byoungchan Lee <byoungchan.lee at gmx.com>
Date: Thu, 10 Oct 2024 18:17:29 +0900
Subject: [PATCH 4/8] Add test cases where the cwd and the compilation
database's directory mismatch
---
clang-tools-extra/include-cleaner/test/tool.cpp | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/clang-tools-extra/include-cleaner/test/tool.cpp b/clang-tools-extra/include-cleaner/test/tool.cpp
index 2155eec189d186..768fc4f4e583fb 100644
--- a/clang-tools-extra/include-cleaner/test/tool.cpp
+++ b/clang-tools-extra/include-cleaner/test/tool.cpp
@@ -38,13 +38,12 @@ int x = foo();
// PRINT: #include "foo.h"
// PRINT-NOT: {{^}}#include "foobar.h"{{$}}
+// RUN: mkdir -p $(dirname %t)/out
// RUN: cp %s %t.cpp
-// RUN: clang-include-cleaner -edit %t.cpp -- -I%S/Inputs/
-// RUN: FileCheck --match-full-lines --check-prefix=EDIT %s < %t.cpp
-// EDIT: #include "foo.h"
-// EDIT-NOT: {{^}}#include "foobar.h"{{$}}
-
-// RUN: cp %s %t.cpp
-// RUN: clang-include-cleaner -edit --ignore-headers="foobar\.h,foo\.h" %t.cpp -- -I%S/Inputs/
-// RUN: FileCheck --match-full-lines --check-prefix=EDIT2 %s < %t.cpp
-// EDIT2-NOT: {{^}}#include "foo.h"{{$}}
+// RUN: echo "[{\"directory\":\"$(dirname %t)/out\",\"file\":\"../$(basename %t).cpp\",\"command\":\":clang++ -I%S/Inputs/ ../$(basename %t).cpp\"}]" > $(dirname %t)/out/compile_commands.json
+// RUN: pushd $(dirname %t)
+// RUN: clang-include-cleaner -p out -edit $(basename %t).cpp
+// RUN: popd
+// RUN: FileCheck --match-full-lines --check-prefix=EDIT3 %s < %t.cpp
+// EDIT3: #include "foo.h"
+// EDIT3-NOT: {{^}}#include "foobar.h"{{$}}
>From dec9be82210162c3399f60c588a82770c602c5f1 Mon Sep 17 00:00:00 2001
From: Byoungchan Lee <byoungchan.lee at gmx.com>
Date: Thu, 10 Oct 2024 18:39:27 +0900
Subject: [PATCH 5/8] Fix the previous commit that removed existing test cases
---
clang-tools-extra/include-cleaner/test/tool.cpp | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/clang-tools-extra/include-cleaner/test/tool.cpp b/clang-tools-extra/include-cleaner/test/tool.cpp
index 768fc4f4e583fb..92e69a807dedd8 100644
--- a/clang-tools-extra/include-cleaner/test/tool.cpp
+++ b/clang-tools-extra/include-cleaner/test/tool.cpp
@@ -38,6 +38,17 @@ int x = foo();
// PRINT: #include "foo.h"
// PRINT-NOT: {{^}}#include "foobar.h"{{$}}
+// RUN: cp %s %t.cpp
+// RUN: clang-include-cleaner -edit %t.cpp -- -I%S/Inputs/
+// RUN: FileCheck --match-full-lines --check-prefix=EDIT %s < %t.cpp
+// EDIT: #include "foo.h"
+// EDIT-NOT: {{^}}#include "foobar.h"{{$}}
+
+// RUN: cp %s %t.cpp
+// RUN: clang-include-cleaner -edit --ignore-headers="foobar\.h,foo\.h" %t.cpp -- -I%S/Inputs/
+// RUN: FileCheck --match-full-lines --check-prefix=EDIT2 %s < %t.cpp
+// EDIT2-NOT: {{^}}#include "foo.h"{{$}}
+
// RUN: mkdir -p $(dirname %t)/out
// RUN: cp %s %t.cpp
// RUN: echo "[{\"directory\":\"$(dirname %t)/out\",\"file\":\"../$(basename %t).cpp\",\"command\":\":clang++ -I%S/Inputs/ ../$(basename %t).cpp\"}]" > $(dirname %t)/out/compile_commands.json
>From b39f36e4e5ef4f6be815dd0cfdbbbab09be81c20 Mon Sep 17 00:00:00 2001
From: Byoungchan Lee <byoungchan.lee at gmx.com>
Date: Thu, 10 Oct 2024 18:43:16 +0900
Subject: [PATCH 6/8] Make clang-format happy
---
clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
index c0363b3d2bdb56..6b1e716e1290f1 100644
--- a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
+++ b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
@@ -174,7 +174,8 @@ class Action : public clang::ASTFrontendAction {
writeHTML();
// Source File's path of compiler invocation, converted to absolute path.
- llvm::SmallString<256> AbsPath(SM.getFileEntryRefForID(SM.getMainFileID())->getName());
+ llvm::SmallString<256> AbsPath(
+ SM.getFileEntryRefForID(SM.getMainFileID())->getName());
assert(!AbsPath.empty() && "Main file path not known?");
SM.getFileManager().makeAbsolutePath(AbsPath);
llvm::StringRef Code = SM.getBufferData(SM.getMainFileID());
>From 3a0f02e461ddcd4c25075d9fc309a7f4335c371f Mon Sep 17 00:00:00 2001
From: Byoungchan Lee <byoungchan.lee at gmx.com>
Date: Thu, 10 Oct 2024 20:03:32 +0900
Subject: [PATCH 7/8] Fix unittest failures in Windows
---
clang-tools-extra/include-cleaner/test/tool.cpp | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/clang-tools-extra/include-cleaner/test/tool.cpp b/clang-tools-extra/include-cleaner/test/tool.cpp
index 92e69a807dedd8..8994bd3e2c5216 100644
--- a/clang-tools-extra/include-cleaner/test/tool.cpp
+++ b/clang-tools-extra/include-cleaner/test/tool.cpp
@@ -49,12 +49,15 @@ int x = foo();
// RUN: FileCheck --match-full-lines --check-prefix=EDIT2 %s < %t.cpp
// EDIT2-NOT: {{^}}#include "foo.h"{{$}}
+// This test has commands that rely on shell capabilities that won't execute
+// correctly on Windows e.g. subshell execution
+// REQUIRES: shell
// RUN: mkdir -p $(dirname %t)/out
// RUN: cp %s %t.cpp
// RUN: echo "[{\"directory\":\"$(dirname %t)/out\",\"file\":\"../$(basename %t).cpp\",\"command\":\":clang++ -I%S/Inputs/ ../$(basename %t).cpp\"}]" > $(dirname %t)/out/compile_commands.json
-// RUN: pushd $(dirname %t)
-// RUN: clang-include-cleaner -p out -edit $(basename %t).cpp
-// RUN: popd
-// RUN: FileCheck --match-full-lines --check-prefix=EDIT3 %s < %t.cpp
-// EDIT3: #include "foo.h"
-// EDIT3-NOT: {{^}}#include "foobar.h"{{$}}
+// RUN: pushd $(dirname %t)
+// RUN: clang-include-cleaner -p out -edit $(basename %t).cpp
+// RUN: popd
+// RUN: FileCheck --match-full-lines --check-prefix=EDIT3 %s < %t.cpp
+// EDIT3: #include "foo.h"
+// EDIT3-NOT: {{^}}#include "foobar.h"{{$}}
>From c65d828596f1845a736e993f5adc0e89d8b7d895 Mon Sep 17 00:00:00 2001
From: Byoungchan Lee <byoungchan.lee at gmx.com>
Date: Sun, 13 Oct 2024 20:24:09 +0900
Subject: [PATCH 8/8] Apply review comments.
- Removed the check for Edit mode.
- Removed unnecessary fallback.
- Various cleanups.
- Make the test work on Windows as well.
---
.../include-cleaner/test/tool.cpp | 11 +--
.../include-cleaner/tool/IncludeCleaner.cpp | 92 ++++++-------------
2 files changed, 31 insertions(+), 72 deletions(-)
diff --git a/clang-tools-extra/include-cleaner/test/tool.cpp b/clang-tools-extra/include-cleaner/test/tool.cpp
index 8994bd3e2c5216..d72d2317ce2b1d 100644
--- a/clang-tools-extra/include-cleaner/test/tool.cpp
+++ b/clang-tools-extra/include-cleaner/test/tool.cpp
@@ -49,14 +49,11 @@ int x = foo();
// RUN: FileCheck --match-full-lines --check-prefix=EDIT2 %s < %t.cpp
// EDIT2-NOT: {{^}}#include "foo.h"{{$}}
-// This test has commands that rely on shell capabilities that won't execute
-// correctly on Windows e.g. subshell execution
-// REQUIRES: shell
-// RUN: mkdir -p $(dirname %t)/out
+// RUN: rm -rf %t.dir && mkdir -p %t.dir
// RUN: cp %s %t.cpp
-// RUN: echo "[{\"directory\":\"$(dirname %t)/out\",\"file\":\"../$(basename %t).cpp\",\"command\":\":clang++ -I%S/Inputs/ ../$(basename %t).cpp\"}]" > $(dirname %t)/out/compile_commands.json
-// RUN: pushd $(dirname %t)
-// RUN: clang-include-cleaner -p out -edit $(basename %t).cpp
+// RUN: echo "[{\"directory\":\"%t.dir\",\"file\":\"../%{t:stem}.tmp.cpp\",\"command\":\":clang++ -I%S/Inputs/ ../%{t:stem}.tmp.cpp\"}]" | sed -e 's/\\/\\\\/g' > %t.dir/compile_commands.json
+// RUN: pushd %t.dir
+// RUN: clang-include-cleaner -p %{t:stem}.tmp.dir -edit ../%{t:stem}.tmp.cpp
// RUN: popd
// RUN: FileCheck --match-full-lines --check-prefix=EDIT3 %s < %t.cpp
// EDIT3: #include "foo.h"
diff --git a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
index 6b1e716e1290f1..a5ecd2cc2339ff 100644
--- a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
+++ b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp
@@ -312,75 +312,37 @@ int main(int argc, const char **argv) {
// CDBToAbsPaths is a map from the path in the compilation database to the
// writable absolute path of the file.
std::map<std::string, std::string> CDBToAbsPaths;
- if (Edit) {
- // if Edit is enabled, `Factory.editedFiles()` will contain the final code,
- // along with the path given in the compilation database. That path can be
- // absolute or relative, and if it is relative, it is relative to the
- // "Directory" field in the compilation database. We need to make it
- // absolute to write the final code to the correct path.
- // There are several cases to consider:
- // 1. The "Directory" field isn't same as the current working directory.
- // 2. The file path resolved from the "Directory" field is not writable.
- // For these cases, we need to find a writable path for the file.
- // To effectively handle these cases, we only need to consider
- // the files from `getSourcePathList()` that are present in the compilation
- // database.
- for (auto &Source : OptionsParser->getSourcePathList()) {
- llvm::SmallString<256> AbsPath(Source);
- if (auto Err = VFS->makeAbsolute(AbsPath)) {
- llvm::errs() << "Failed to get absolute path for " << Source << " : "
- << Err.message() << '\n';
- return 1;
- }
- std::vector<clang::tooling::CompileCommand> Cmds =
- CDB.getCompileCommands(AbsPath);
+ // if Edit is enabled, `Factory.editedFiles()` will contain the final code,
+ // along with the path given in the compilation database. That path can be
+ // absolute or relative, and if it is relative, it is relative to the
+ // "Directory" field in the compilation database. We need to make it
+ // absolute to write the final code to the correct path.
+ for (auto &Source : OptionsParser->getSourcePathList()) {
+ llvm::SmallString<256> AbsPath(Source);
+ if (auto Err = VFS->makeAbsolute(AbsPath)) {
+ llvm::errs() << "Failed to get absolute path for " << Source << " : "
+ << Err.message() << '\n';
+ return 1;
+ }
+ std::vector<clang::tooling::CompileCommand> Cmds =
+ CDB.getCompileCommands(AbsPath);
+ if (Cmds.empty()) {
+ // Try with the original path.
+ Cmds = CDB.getCompileCommands(Source);
if (Cmds.empty()) {
- // Try with the original path.
- Cmds = CDB.getCompileCommands(Source);
- if (Cmds.empty()) {
- continue;
- }
+ // It should be found in the compilation database, even user didn't
+ // specify the compilation database, the `FixedCompilationDatabase` will
+ // create an entry from the arguments. So it is an error if we can't
+ // find the compile commands.
+ llvm::errs() << "No compile commands found for " << Source << '\n';
+ return 1;
}
- // We only need the first command to get the directory.
- auto Cmd = Cmds[0];
+ }
+ for (const auto &Cmd : Cmds) {
llvm::SmallString<256> CDBPath(Cmd.Filename);
std::string Directory(Cmd.Directory);
-
- if (llvm::sys::path::is_absolute(CDBPath)) {
- // If the path in the compilation database is already absolute, we don't
- // need to do anything.
- CDBToAbsPaths[static_cast<std::string>(CDBPath)] =
- static_cast<std::string>(AbsPath);
- } else {
- auto Sept = llvm::sys::path::get_separator();
- // First, try to find the file based on the compilation database.
- llvm::Twine FilePathTwine = Directory + Sept + CDBPath;
- llvm::SmallString<256> FilePath;
- FilePathTwine.toVector(FilePath);
- // Check if it is writable.
- if (llvm::sys::fs::access(FilePath, llvm::sys::fs::AccessMode::Write) !=
- std::error_code()) {
- // If not, try to find the file based on the current working
- // directory, as some Bazel invocations may not set the compilation
- // invocation's filesystem as non-writable. In such cases, we can
- // find the file based on the current working directory.
- FilePath = Source;
- if (auto EC = VFS->makeAbsolute(CDBPath)) {
- llvm::errs() << "Failed to get absolute path for " << CDBPath
- << " : " << EC.message() << '\n';
- return 1;
- }
- if (llvm::sys::fs::access(FilePath,
- llvm::sys::fs::AccessMode::Write) !=
- std::error_code()) {
- llvm::errs() << "Failed to find a writable path for " << Source
- << '\n';
- return 1;
- }
- }
- CDBToAbsPaths[static_cast<std::string>(CDBPath)] =
- static_cast<std::string>(FilePath);
- }
+ llvm::sys::fs::make_absolute(Cmd.Directory, CDBPath);
+ CDBToAbsPaths[std::string(CDBPath)] = std::string(AbsPath);
}
}
More information about the cfe-commits
mailing list