r194250 - clang-format: Write files atomically

[Daniel Jasper]

The patch itself looks good (I don't know why we didn't do that in the
first place).


On Fri, Nov 8, 2013 at 12:07 AM, Alp Toker <alp at nuanti.com> wrote:

> Author: alp
> Date: Fri Nov  8 02:07:19 2013
> New Revision: 194250
>
> URL: http://llvm.org/viewvc/llvm-project?rev=194250&view=rev
> Log:
> clang-format: Write files atomically
>
> Switch clang-format over to Rewriter::overwriteChangedFiles().
>
> The previous implementation was attempting to stream back directly to the
> original file and failing if it was already memory mapped by MemoryBuffer,
> an operation unsupported by Windows.
>
> MemoryBuffer generally mmaps files larger than the physical page size so
> this will have been difficult to reproduce consistently.
>
> This change also reduces flicker in code editors and IDEs on all platforms
> when reformatting in-place.
>

I think it is generally a bad idea for IDEs to use in-place reformatting,
but ok ..

Note that other incorrect uses of MemoryBuffer exist in LLVM/clang and
> will need a similar fix.
>
> A test should be added for Windows when libFormat performance issues are
> fixed (it takes longer than a day to format a 1MB file at present!)
>

Is it the write that takes so long (i.e. fixed with this patch)?

If not, can the file be shared (or changed enough so that it can be shared)?

Modified:
>     cfe/trunk/tools/clang-format/ClangFormat.cpp
>
> Modified: cfe/trunk/tools/clang-format/ClangFormat.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/tools/clang-format/ClangFormat.cpp?rev=194250&r1=194249&r2=194250&view=diff
>
> ==============================================================================
> --- cfe/trunk/tools/clang-format/ClangFormat.cpp (original)
> +++ cfe/trunk/tools/clang-format/ClangFormat.cpp Fri Nov  8 02:07:19 2013
> @@ -213,18 +213,8 @@ static bool format(std::string FileName)
>      Rewriter Rewrite(Sources, LangOptions());
>      tooling::applyAllReplacements(Replaces, Rewrite);
>      if (Inplace) {
> -      if (Replaces.size() == 0)
> -        return false; // Nothing changed, don't touch the file.
> -
> -      std::string ErrorInfo;
> -      llvm::raw_fd_ostream FileStream(FileName.c_str(), ErrorInfo,
> -                                      llvm::sys::fs::F_Binary);
> -      if (!ErrorInfo.empty()) {
> -        llvm::errs() << "Error while writing file: " << ErrorInfo << "\n";
> +      if (Rewrite.overwriteChangedFiles())
>          return true;
> -      }
> -      Rewrite.getEditBuffer(ID).write(FileStream);
> -      FileStream.flush();
>      } else {
>        if (Cursor.getNumOccurrences() != 0)
>          outs() << "{ \"Cursor\": " << tooling::shiftedCodePosition(
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131108/c99867f4/attachment.html>