On May 11, 2012, at 4:33 AM, Manuel Klimek <[email protected]> wrote:
> On Tue, May 8, 2012 at 11:49 AM, Manuel Klimek <[email protected]> wrote: >> On Thu, May 3, 2012 at 6:40 PM, Manuel Klimek <[email protected]> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
