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 <[email protected]> wrote: > On Tue, May 15, 2012 at 8:39 PM, Douglas Gregor <[email protected]> wrote: >> >> 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. > > 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; > +} _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
