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

[Douglas Gregor]

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.

> Second, the testing overhead; note that the RewriterTestContext will
> also be used in a subsequent CL which is currently under review, which
> is why I pulled out a class.

Okay.

Everything else LGTM, thanks!

	- Doug