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

[Douglas Gregor]

On May 22, 2012, at 8:42 AM, Manuel Klimek <klimek at google.com> wrote:

> Do you want to take another round on this? Chandler had the theory
> that the "Otherwise LGTM" means that I can check in once I think I
> addressed your comments - is that generally true, or do you prefer an
> extra cycle?

Sorry for not being clear. Yes, go ahead and check in once you think you've addressed the review comments.

	- Doug

> Thanks,
> /Manuel
> 
> On Wed, May 16, 2012 at 11:16 PM, Manuel Klimek <klimek at google.com> wrote:
>> 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;
>> +}