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

[Manuel Klimek]

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?

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;
> +}