From 23b90bba12c010e5882e09e9f6b765a7281324aa Mon Sep 17 00:00:00 2001 From: Byoungchan Lee Date: Mon, 7 Oct 2024 22:19:38 +0900 Subject: [PATCH 1/9] [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 080099adc9a07..1cb8b9c4ae99b 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 Date: Wed, 9 Oct 2024 19:43:02 +0900 Subject: [PATCH 2/9] 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 1cb8b9c4ae99b..4481601140e3f 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 headerFilter() { }; } +llvm::ErrorOr getCurrentWorkingDirectory() { + llvm::SmallString<256> CurrentPath; + if (const std::error_code EC = llvm::sys::fs::current_path(CurrentPath)) + return llvm::ErrorOr(EC); + return std::string(CurrentPath.str()); +} + +std::optional +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(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 EditedFilesToActualFiles; + if (Edit) { + std::vector 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 Date: Thu, 10 Oct 2024 18:02:27 +0900 Subject: [PATCH 3/9] 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 4481601140e3f..c0363b3d2bdb5 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 headerFilter() { }; } -llvm::ErrorOr getCurrentWorkingDirectory() { - llvm::SmallString<256> CurrentPath; - if (const std::error_code EC = llvm::sys::fs::current_path(CurrentPath)) - return llvm::ErrorOr(EC); - return std::string(CurrentPath.str()); -} - -std::optional -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(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 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 CDBToAbsPaths; if (Edit) { - std::vector 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 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(CDBPath)] = + static_cast(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(CDBPath)] = + static_cast(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 Date: Thu, 10 Oct 2024 18:17:29 +0900 Subject: [PATCH 4/9] 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 2155eec189d18..768fc4f4e583f 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 Date: Thu, 10 Oct 2024 18:39:27 +0900 Subject: [PATCH 5/9] 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 768fc4f4e583f..92e69a807dedd 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 Date: Thu, 10 Oct 2024 18:43:16 +0900 Subject: [PATCH 6/9] 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 c0363b3d2bdb5..6b1e716e1290f 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 Date: Thu, 10 Oct 2024 20:03:32 +0900 Subject: [PATCH 7/9] 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 92e69a807dedd..8994bd3e2c521 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 Date: Sun, 13 Oct 2024 20:24:09 +0900 Subject: [PATCH 8/9] 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 8994bd3e2c521..d72d2317ce2b1 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 6b1e716e1290f..a5ecd2cc2339f 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 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 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 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(CDBPath)] = - static_cast(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(CDBPath)] = - static_cast(FilePath); - } + llvm::sys::fs::make_absolute(Cmd.Directory, CDBPath); + CDBToAbsPaths[std::string(CDBPath)] = std::string(AbsPath); } } From 11f78a32c4d1d2c6b651b7c4e753ac2b54c85e9e Mon Sep 17 00:00:00 2001 From: Byoungchan Lee Date: Mon, 14 Oct 2024 23:45:09 +0900 Subject: [PATCH 9/9] Apply review comments - Removed unnecessary mention of Edit - Removed fallback logic when the absolute path isn't found - Extracted mapInputsToAbsPaths as a separate function --- .../include-cleaner/tool/IncludeCleaner.cpp | 84 +++++++++++-------- 1 file changed, 48 insertions(+), 36 deletions(-) diff --git a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp index a5ecd2cc2339f..6bd9c40c70753 100644 --- a/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp +++ b/clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp @@ -282,6 +282,48 @@ std::function headerFilter() { }; } +// Maps absolute path of each files of each compilation commands to the +// absolute path of the input file. +llvm::Expected> +mapInputsToAbsPaths(clang::tooling::CompilationDatabase &CDB, + llvm::IntrusiveRefCntPtr VFS, + const std::vector &Inputs) { + std::map CDBToAbsPaths; + // 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 : Inputs) { + llvm::SmallString<256> AbsPath(Source); + if (auto Err = VFS->makeAbsolute(AbsPath)) { + llvm::errs() << "Failed to get absolute path for " << Source << " : " + << Err.message() << '\n'; + return std::move(llvm::errorCodeToError(Err)); + } + std::vector Cmds = + CDB.getCompileCommands(AbsPath); + if (Cmds.empty()) { + // 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. + std::string ErrorMsg = + llvm::formatv("No compile commands found for {0}", AbsPath).str(); + llvm::errs() << ErrorMsg << '\n'; + return llvm::make_error( + ErrorMsg, llvm::inconvertibleErrorCode()); + } + for (const auto &Cmd : Cmds) { + llvm::SmallString<256> CDBPath(Cmd.Filename); + std::string Directory(Cmd.Directory); + llvm::sys::fs::make_absolute(Cmd.Directory, CDBPath); + CDBToAbsPaths[std::string(CDBPath)] = std::string(AbsPath); + } + } + return CDBToAbsPaths; +} + } // namespace } // namespace include_cleaner } // namespace clang @@ -311,40 +353,10 @@ int main(int argc, const char **argv) { 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 CDBToAbsPaths; - // 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 Cmds = - CDB.getCompileCommands(AbsPath); - if (Cmds.empty()) { - // Try with the original path. - Cmds = CDB.getCompileCommands(Source); - if (Cmds.empty()) { - // 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; - } - } - for (const auto &Cmd : Cmds) { - llvm::SmallString<256> CDBPath(Cmd.Filename); - std::string Directory(Cmd.Directory); - llvm::sys::fs::make_absolute(Cmd.Directory, CDBPath); - CDBToAbsPaths[std::string(CDBPath)] = std::string(AbsPath); - } - } + auto CDBToAbsPaths = + mapInputsToAbsPaths(CDB, VFS, OptionsParser->getSourcePathList()); + if (!CDBToAbsPaths) + return 1; clang::tooling::ClangTool Tool(CDB, OptionsParser->getSourcePathList()); @@ -356,8 +368,8 @@ int main(int argc, const char **argv) { if (Edit) { for (const auto &NameAndContent : Factory.editedFiles()) { llvm::StringRef FileName = NameAndContent.first(); - if (auto It = CDBToAbsPaths.find(FileName.str()); - It != CDBToAbsPaths.end()) + if (auto It = CDBToAbsPaths->find(FileName.str()); + It != CDBToAbsPaths->end()) FileName = It->second; const std::string &FinalCode = NameAndContent.second;