-
Notifications
You must be signed in to change notification settings - Fork 10.8k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[clang][tidy] Ensure rewriter has the correct CWD (#67839)
This patch replaces use of the deprecated `FileEntry::getName()` with `FileEntryRef::getName()`. This means the code now uses the path that was used to register file entry in `SourceManager` instead of the absolute path that happened to be used in the last call to `FileManager::getFile()` some place else. This caused some test failures due to the fact that some paths can be relative and thus rely on the VFS CWD. The CWD can change for each TU, so when we run `clang-tidy` on a compilation database and try to perform all the replacements at the end, relative paths won't resolve the same. This patch takes care to reinstate the correct CWD and make the path reported by `FileEntryRef` absolute before passing it to `llvm::writeToOutput()`.
- Loading branch information
1 parent
9f87509
commit 07157db
Showing
4 changed files
with
44 additions
and
18 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -147,7 +147,8 @@ class ErrorReporter { | |
Files.makeAbsolutePath(FixAbsoluteFilePath); | ||
tooling::Replacement R(FixAbsoluteFilePath, Repl.getOffset(), | ||
Repl.getLength(), Repl.getReplacementText()); | ||
Replacements &Replacements = FileReplacements[R.getFilePath()]; | ||
auto &Entry = FileReplacements[R.getFilePath()]; | ||
Replacements &Replacements = Entry.Replacements; | ||
llvm::Error Err = Replacements.add(R); | ||
if (Err) { | ||
// FIXME: Implement better conflict handling. | ||
|
@@ -174,6 +175,7 @@ class ErrorReporter { | |
} | ||
FixLoc = getLocation(FixAbsoluteFilePath, Repl.getOffset()); | ||
FixLocations.push_back(std::make_pair(FixLoc, CanBeApplied)); | ||
Entry.BuildDir = Error.BuildDirectory; | ||
} | ||
} | ||
} | ||
|
@@ -189,9 +191,14 @@ class ErrorReporter { | |
|
||
void finish() { | ||
if (TotalFixes > 0) { | ||
Rewriter Rewrite(SourceMgr, LangOpts); | ||
auto &VFS = Files.getVirtualFileSystem(); | ||
auto OriginalCWD = VFS.getCurrentWorkingDirectory(); | ||
bool AnyNotWritten = false; | ||
|
||
for (const auto &FileAndReplacements : FileReplacements) { | ||
Rewriter Rewrite(SourceMgr, LangOpts); | ||
StringRef File = FileAndReplacements.first(); | ||
VFS.setCurrentWorkingDirectory(FileAndReplacements.second.BuildDir); | ||
llvm::ErrorOr<std::unique_ptr<MemoryBuffer>> Buffer = | ||
SourceMgr.getFileManager().getBufferForFile(File); | ||
if (!Buffer) { | ||
|
@@ -208,8 +215,8 @@ class ErrorReporter { | |
continue; | ||
} | ||
llvm::Expected<tooling::Replacements> Replacements = | ||
format::cleanupAroundReplacements(Code, FileAndReplacements.second, | ||
*Style); | ||
format::cleanupAroundReplacements( | ||
Code, FileAndReplacements.second.Replacements, *Style); | ||
if (!Replacements) { | ||
llvm::errs() << llvm::toString(Replacements.takeError()) << "\n"; | ||
continue; | ||
|
@@ -226,13 +233,18 @@ class ErrorReporter { | |
if (!tooling::applyAllReplacements(Replacements.get(), Rewrite)) { | ||
llvm::errs() << "Can't apply replacements for file " << File << "\n"; | ||
} | ||
AnyNotWritten &= Rewrite.overwriteChangedFiles(); | ||
} | ||
if (Rewrite.overwriteChangedFiles()) { | ||
|
||
if (AnyNotWritten) { | ||
This comment has been minimized.
Sorry, something went wrong.
This comment has been minimized.
Sorry, something went wrong.
jansvoboda11
Author
Contributor
|
||
llvm::errs() << "clang-tidy failed to apply suggested fixes.\n"; | ||
} else { | ||
llvm::errs() << "clang-tidy applied " << AppliedFixes << " of " | ||
<< TotalFixes << " suggested fixes.\n"; | ||
} | ||
|
||
if (OriginalCWD) | ||
VFS.setCurrentWorkingDirectory(*OriginalCWD); | ||
} | ||
} | ||
|
||
|
@@ -289,13 +301,18 @@ class ErrorReporter { | |
return CharSourceRange::getCharRange(BeginLoc, EndLoc); | ||
} | ||
|
||
struct ReplacementsWithBuildDir { | ||
StringRef BuildDir; | ||
Replacements Replacements; | ||
}; | ||
|
||
FileManager Files; | ||
LangOptions LangOpts; // FIXME: use langopts from each original file | ||
IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts; | ||
DiagnosticConsumer *DiagPrinter; | ||
DiagnosticsEngine Diags; | ||
SourceManager SourceMgr; | ||
llvm::StringMap<Replacements> FileReplacements; | ||
llvm::StringMap<ReplacementsWithBuildDir> FileReplacements; | ||
ClangTidyContext &Context; | ||
FixBehaviour ApplyFixes; | ||
unsigned TotalFixes = 0U; | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
@jansvoboda11 static verifier reports that AnyNotWritten is always false since it is initialized to false and only assigned with &=. Was |= intended here or something else?