[cfe-commits] [PATCH] Add OverwriteChangedFiles to the Rewriter

[Manuel Klimek]

On Tue, May 15, 2012 at 8:39 PM, Douglas Gregor <dgregor at apple.com> wrote:
>
> On May 11, 2012, at 4:33 AM, Manuel Klimek <klimek at google.com> wrote:
>
>> On Tue, May 8, 2012 at 11:49 AM, Manuel Klimek <klimek at google.com> wrote:
>>> On Thu, May 3, 2012 at 6:40 PM, Manuel Klimek <klimek at google.com> wrote:
>>>> as requested by Doug in the clangRefactoring review... in addition,
>>>> this patch adds a unit test for the Rewriter (into Tooling/ so we
>>>> don't link yet another unit test), and pulls out a RewriterTestContext
>>>> which I need for clangRefactoring when it's checked in - but it's also
>>>> really nice to unit test Rewriter functionality...
>>
>> Inlining the patch in the hope that that speeds up review :)
>>
>> First, the changed code:
>>
>> Index: include/clang/Rewrite/Rewriter.h
>> ===================================================================
>> --- include/clang/Rewrite/Rewriter.h    (revision 156059)
>> +++ include/clang/Rewrite/Rewriter.h    (working copy)
>> @@ -279,6 +279,13 @@
>>   buffer_iterator buffer_begin() { return RewriteBuffers.begin(); }
>>   buffer_iterator buffer_end() { return RewriteBuffers.end(); }
>>
>> +  /// SaveFiles - Save all changed files to disk.
>> +  ///
>> +  /// Returns whether all changes were saves successfully.
>> +  /// Outputs diagnostics via the source manager's diagnostic engine
>> +  /// in case of an error.
>> +  bool OverwriteChangedFiles();
>> +
>
> I think that's spelled:
>
>  bool overwriteChangedFiles();
>
> Also, we usually use a return of "true" to indicate that there was an error.
>
>
>> private:
>>   unsigned getLocationOffsetAndFileID(SourceLocation Loc, FileID &FID) const;
>> };
>> Index: lib/Rewrite/Rewriter.cpp
>> ===================================================================
>> --- lib/Rewrite/Rewriter.cpp    (revision 156059)
>> +++ lib/Rewrite/Rewriter.cpp    (working copy)
>> @@ -15,8 +15,10 @@
>> #include "clang/Rewrite/Rewriter.h"
>> #include "clang/AST/Stmt.h"
>> #include "clang/AST/Decl.h"
>> +#include "clang/Basic/FileManager.h"
>> +#include "clang/Basic/SourceManager.h"
>> #include "clang/Lex/Lexer.h"
>> -#include "clang/Basic/SourceManager.h"
>> +#include "clang/Frontend/FrontendDiagnostic.h"
>> #include "llvm/ADT/SmallString.h"
>> using namespace clang;
>>
>> @@ -412,3 +414,23 @@
>>
>>   return false;
>> }
>> +
>> +bool Rewriter::OverwriteChangedFiles() {
>> +  bool AllWritten = true;
>> +  for (buffer_iterator I = buffer_begin(), E = buffer_end(); I != E; ++I) {
>> +    const FileEntry *Entry =
>> +        getSourceMgr().getFileEntryForID(I->first);
>> +    std::string ErrorInfo;
>> +    llvm::raw_fd_ostream FileStream(
>> +        Entry->getName(), ErrorInfo, llvm::raw_fd_ostream::F_Binary);
>> +    if (!ErrorInfo.empty()) {
>> +      getSourceMgr().getDiagnostics().Report(clang::diag::err_fe_unable_to_open_output)
>> +        << Entry->getName() << ErrorInfo;
>> +      AllWritten = false;
>> +      continue;
>> +    }
>> +    I->second.write(FileStream);
>> +    FileStream.flush();
>> +  }
>> +  return AllWritten;
>> +}
>
> It would be better to write the data to a temporary file and then rename it, like we do with other outputs.

All done. PTAL.

Thanks!
/Manuel

Index: include/clang/Rewrite/Rewriter.h
===================================================================
--- include/clang/Rewrite/Rewriter.h    (revision 156947)
+++ include/clang/Rewrite/Rewriter.h    (working copy)
@@ -279,6 +279,13 @@
   buffer_iterator buffer_begin() { return RewriteBuffers.begin(); }
   buffer_iterator buffer_end() { return RewriteBuffers.end(); }

+  /// SaveFiles - Save all changed files to disk.
+  ///
+  /// Returns whether not all changes were saved successfully.
+  /// Outputs diagnostics via the source manager's diagnostic engine
+  /// in case of an error.
+  bool overwriteChangedFiles();
+
 private:
   unsigned getLocationOffsetAndFileID(SourceLocation Loc, FileID &FID) const;
 };
Index: lib/Rewrite/Rewriter.cpp
===================================================================
--- lib/Rewrite/Rewriter.cpp    (revision 156947)
+++ lib/Rewrite/Rewriter.cpp    (working copy)
@@ -15,9 +15,12 @@
 #include "clang/Rewrite/Rewriter.h"
 #include "clang/AST/Stmt.h"
 #include "clang/AST/Decl.h"
+#include "clang/Basic/DiagnosticIDs.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Basic/SourceManager.h"
 #include "clang/Lex/Lexer.h"
-#include "clang/Basic/SourceManager.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/Support/FileSystem.h"
 using namespace clang;

 raw_ostream &RewriteBuffer::write(raw_ostream &os) const {
@@ -412,3 +415,68 @@

   return false;
 }
+
+// A wrapper for a file stream that atomically overwrites the target.
+//
+// Creates a file output stream for a temporary file in the constructor,
+// which is later accessible via getStream() if ok() return true.
+// Flushes the stream and moves the temporary file to the target location
+// in the destructor.
+class AtomicallyMovedFile {
+public:
+  AtomicallyMovedFile(DiagnosticsEngine &Diagnostics, StringRef Filename,
+                      bool &AllWritten)
+    : Diagnostics(Diagnostics), Filename(Filename), AllWritten(AllWritten) {
+    TempFilename = Filename;
+    TempFilename += "-%%%%%%%%";
+    int FD;
+    if (llvm::sys::fs::unique_file(TempFilename.str(), FD, TempFilename,
+                                    /*makeAbsolute=*/true, 0664)) {
+      AllWritten = false;
+      Diagnostics.Report(clang::diag::err_unable_to_make_temp)
+        << TempFilename;
+    } else {
+      FileStream.reset(new llvm::raw_fd_ostream(FD, /*shouldClose=*/true));
+    }
+  }
+
+  ~AtomicallyMovedFile() {
+    if (!ok()) return;
+
+    FileStream->flush();
+    if (llvm::error_code ec =
+          llvm::sys::fs::rename(TempFilename.str(), Filename)) {
+      AllWritten = false;
+      Diagnostics.Report(clang::diag::err_unable_to_rename_temp)
+        << TempFilename << Filename << ec.message();
+      bool existed;
+      // If the remove fails, there's not a lot we can do - this is already an
+      // error.
+      llvm::sys::fs::remove(TempFilename.str(), existed);
+    }
+  }
+
+  bool ok() { return FileStream; }
+  llvm::raw_ostream &getStream() { return *FileStream; }
+
+private:
+  DiagnosticsEngine &Diagnostics;
+  StringRef Filename;
+  SmallString<128> TempFilename;
+  OwningPtr<llvm::raw_fd_ostream> FileStream;
+  bool &AllWritten;
+};
+
+bool Rewriter::overwriteChangedFiles() {
+  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());
+    }
+  }
+  return !AllWritten;
+}