-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[clang-include-cleaner] Fix incorrect directory issue for writing files #111375
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
23b90bb
eef397c
2409d6f
0c9477d
dec9be8
b39f36e
3a0f02e
c65d828
11f78a3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,3 +48,16 @@ int x = foo(); | |
| // 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"{{$}} | ||
|
|
||
| // 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"{{$}} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -173,9 +173,11 @@ class Action : public clang::ASTFrontendAction { | |
| if (!HTMLReportPath.empty()) | ||
| writeHTML(); | ||
|
|
||
| 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 = | ||
|
|
@@ -185,7 +187,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) { | ||
|
|
@@ -202,7 +204,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() { | ||
|
|
@@ -305,8 +307,84 @@ int main(int argc, const char **argv) { | |
| } | ||
| } | ||
|
|
||
| clang::tooling::ClangTool Tool(OptionsParser->getCompilations(), | ||
| OptionsParser->getSourcePathList()); | ||
| 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) { | ||
|
||
| // 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 (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(CDB, OptionsParser->getSourcePathList()); | ||
|
|
||
| auto HeaderFilter = headerFilter(); | ||
| if (!HeaderFilter) | ||
|
|
@@ -316,6 +394,10 @@ 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()) | ||
| FileName = It->second; | ||
|
|
||
| const std::string &FinalCode = NameAndContent.second; | ||
| if (auto Err = llvm::writeToOutput( | ||
| FileName, [&](llvm::raw_ostream &OS) -> llvm::Error { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't just affect that particular run, affects the whole, can you move the test into its own file ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was explained in the previous comment. It now works on Windows as well.