thakis created this revision.
Herald added a subscriber: klimek.
Second attempt after http://llvm.org/viewvc/llvm-project?rev=296166&view=rev
In the first attempt, `Code` (the memory buffer backing the input file) was
reset before `overwriteChangedFiles()` was called, but
`overwriteChangedFiles()` still reads from it. Now, give
`overwriteChangedFiles()` a callback that's called after the input's been read
but before the output's written, and then call `Code.reset()` from that
callback.
(Since the test is identical to what was in the repo before chapuni's revert,
`svn diff` doesn't show it – see the above link for the test.)
https://reviews.llvm.org/D30385
Files:
include/clang/Rewrite/Core/Rewriter.h
lib/Frontend/Rewrite/FixItRewriter.cpp
lib/Rewrite/Rewriter.cpp
lib/Tooling/Refactoring.cpp
test/Format/inplace.cpp
tools/clang-format/ClangFormat.cpp
unittests/Tooling/RefactoringTest.cpp
unittests/Tooling/RewriterTest.cpp
Index: tools/clang-format/ClangFormat.cpp
===================================================================
--- tools/clang-format/ClangFormat.cpp
+++ tools/clang-format/ClangFormat.cpp
@@ -299,7 +299,10 @@
if (Inplace) {
if (FileName == "-")
errs() << "error: cannot use -i when reading from stdin.\n";
- else if (Rewrite.overwriteChangedFiles())
+ else if (Rewrite.overwriteChangedFiles([&Code, ID](FileID FID) {
+ assert(FID == ID);
+ Code.reset();
+ }))
return true;
} else {
if (Cursor.getNumOccurrences() != 0)
Index: lib/Tooling/Refactoring.cpp
===================================================================
--- lib/Tooling/Refactoring.cpp
+++ lib/Tooling/Refactoring.cpp
@@ -64,7 +64,9 @@
}
int RefactoringTool::saveRewrittenFiles(Rewriter &Rewrite) {
- return Rewrite.overwriteChangedFiles() ? 1 : 0;
+ // FIXME: This likely leaks temp files on Windows, see comment on
+ // Rewriter::overwriteChangedFiles().
+ return Rewrite.overwriteChangedFiles([](FileID){}) ? 1 : 0;
}
bool formatAndApplyAllReplacements(
Index: lib/Frontend/Rewrite/FixItRewriter.cpp
===================================================================
--- lib/Frontend/Rewrite/FixItRewriter.cpp
+++ lib/Frontend/Rewrite/FixItRewriter.cpp
@@ -82,9 +82,9 @@
Editor.applyRewrites(Rec);
if (FixItOpts->InPlace) {
- // Overwriting open files on Windows is tricky, but the rewriter can do it
- // for us.
- Rewrite.overwriteChangedFiles();
+ // FIXME: This likely leaks temp files on Windows, see comment on
+ // Rewriter::overwriteChangedFiles().
+ Rewrite.overwriteChangedFiles([](FileID){});
return false;
}
Index: lib/Rewrite/Rewriter.cpp
===================================================================
--- lib/Rewrite/Rewriter.cpp
+++ lib/Rewrite/Rewriter.cpp
@@ -443,15 +443,16 @@
};
} // end anonymous namespace
-bool Rewriter::overwriteChangedFiles() {
+bool Rewriter::overwriteChangedFiles(llvm::function_ref<void(FileID)> prewrite) {
bool AllWritten = true;
for (buffer_iterator I = buffer_begin(), E = buffer_end(); I != E; ++I) {
const FileEntry *Entry =
getSourceMgr().getFileEntryForID(I->first);
AtomicallyMovedFile File(getSourceMgr().getDiagnostics(), Entry->getName(),
AllWritten);
if (File.ok()) {
I->second.write(File.getStream());
+ prewrite(I->first);
}
}
return !AllWritten;
Index: include/clang/Rewrite/Core/Rewriter.h
===================================================================
--- include/clang/Rewrite/Core/Rewriter.h
+++ include/clang/Rewrite/Core/Rewriter.h
@@ -184,7 +184,18 @@
/// Returns true if any files were not saved successfully.
/// Outputs diagnostics via the source manager's diagnostic engine
/// in case of an error.
- bool overwriteChangedFiles();
+ ///
+ /// prewrite(FID) is called after all changes to FID have been written to a
+ /// temporary file, but before that temporary file is moved into FID's
+ /// location. Windows's ReplaceFileW(to, from) internally creates (another)
+ /// temporary file and swaps the data streams of that file with the one of
+ /// |to| so that it's possible to replace an opened file. However, if |to|
+ /// _was_ opened, the temp file now has that open data stream, and
+ /// ReplaceFileW() fails to delete it, causing a tempo file leak. To fix,
+ /// clients of this function must close all file mappings of a file before
+ /// it's written to (but after it's been read by Rewriter), and this prewrite
+ /// callback is a convenient place for doing this.
+ bool overwriteChangedFiles(llvm::function_ref<void(FileID)> prewrite);
private:
unsigned getLocationOffsetAndFileID(SourceLocation Loc, FileID &FID) const;
Index: unittests/Tooling/RewriterTest.cpp
===================================================================
--- unittests/Tooling/RewriterTest.cpp
+++ unittests/Tooling/RewriterTest.cpp
@@ -19,7 +19,9 @@
RewriterTestContext Context;
FileID ID = Context.createOnDiskFile("t.cpp", "line1\nline2\nline3\nline4");
Context.Rewrite.ReplaceText(Context.getLocation(ID, 2, 1), 5, "replaced");
- EXPECT_FALSE(Context.Rewrite.overwriteChangedFiles());
+ // This test doesn't create file mappings, so overwriteChangedFiles() needs
+ // no parameter.
+ EXPECT_FALSE(Context.Rewrite.overwriteChangedFiles([](FileID){}));
EXPECT_EQ("line1\nreplaced\nline3\nline4",
Context.getFileContentFromDisk("t.cpp"));
}
@@ -32,7 +34,9 @@
"working.cpp", "line1\nline2\nline3\nline4");
Context.Rewrite.ReplaceText(Context.getLocation(WorkingID, 2, 1), 5,
"replaced");
- EXPECT_TRUE(Context.Rewrite.overwriteChangedFiles());
+ // This test doesn't create file mappings, so overwriteChangedFiles() needs
+ // no parameter.
+ EXPECT_TRUE(Context.Rewrite.overwriteChangedFiles([](FileID){}));
EXPECT_EQ("line1\nreplaced\nline3\nline4",
Context.getFileContentFromDisk("working.cpp"));
}
Index: unittests/Tooling/RefactoringTest.cpp
===================================================================
--- unittests/Tooling/RefactoringTest.cpp
+++ unittests/Tooling/RefactoringTest.cpp
@@ -639,7 +639,9 @@
Replacements Replaces = toReplacements({Replacement(
Context.Sources, Context.getLocation(ID, 2, 1), 5, "replaced")});
EXPECT_TRUE(applyAllReplacements(Replaces, Context.Rewrite));
- EXPECT_FALSE(Context.Rewrite.overwriteChangedFiles());
+ // This test doesn't create file mappings, so overwriteChangedFiles() needs
+ // no parameter.
+ EXPECT_FALSE(Context.Rewrite.overwriteChangedFiles([](FileID){}));
EXPECT_EQ("line1\nreplaced\nline3\nline4",
getFileContentFromDisk("input.cpp"));
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
