[cfe-dev] Help solve bug 17216 for clang-format on Windows

[Daniel Jasper]

On Sun, Nov 3, 2013 at 2:19 PM, Johan Engelen <jbc.engelen at swissonline.ch>wrote:

>  On 2-11-2013 23:16, Alp Toker wrote:
>
> On 02/11/2013 20:48, Johan Engelen wrote:
>
> Hi Alp,
>   The patch solves the problem, thanks!
>
>
> Glad to hear it!
>
>
> One comment: the format function returns 'false' when no errors occurred.
> So I believe it should read
>       if (Rewrite.overwriteChangedFiles()) {
>         return false;
>       }
>
>
> Disagree. My patch is fine :-)
>
> It's just that the return value of overwriteChangedFiles() was
> mis-documented until my fix last week (r193594) so that might have caused
> confusion.
>
> But I'll appreciate if you can confirm:
>
>    /// overwriteChangedFiles - Save all changed files to disk.
>    ///
>    /// Returns true if any files were not saved successfully.
>    /// Outputs diagnostics via the source manager's diagnostic engine
>    /// in case of an error.
>    bool overwriteChangedFiles();
>
> // Returns true on error.
>  static bool format(std::string FileName) {
>  ...
>        if (Rewrite.overwriteChangedFiles())
>          return true;
>  ...
>  }
>
>
> Agreed :-)
>
>
>
> To land this fix we'll need a test case. I don't want to check in a 16k+
> source file into SVN so we'll have to either generate a test input or use
> one of the existing large file inputs for that.
>
>
> (I'm sorry, I'm not sure how to help with this. It's been only 3 days
> since I've first seen the codebase, so... :-)
> Adding a test case means adding to /llvm/tools/clang/test/Format ?
> I'll see if I can figure it out from the other tests in the codebase.
>

Yes, a test like you are describing needs to go in clang/test/Format (which
mostly does integration tests of the clang-format binary). There is also
clang/unittests/Format, but those test specific formatting aspects of stuff
in clang/lib/Format.

I think you should just generate a 16kB file (doesn't have to contain valid
C++ .. basically any 16k characters should do). Or just check in a 16kB
file, there are a lot of tests of that size ..

Cheers,
Daniel

-Johan
>
>
>
>  Alp.
>
>
>
>
> Cheers,
>   Johan
>
>
> On 2-11-2013 21:05, Alp Toker wrote:
>
> Hello Johan,
>
> I've attached a patch I'm using locally to reduce flicker in my IDE
> while running clang-format using atomic file save.
>
> I suspect it resolves you issue too, could you try it and report back?
>
> Alp.
>
>
> On 01/11/2013 21:12, Johan Engelen wrote:
>
> Hello all,
>    I've been looking into why clang-format cannot write in-place for
> large files (> 16 kB) on Windows [1].
> I've found the culprit, but need help actually fixing it; the file
> handling code is somewhat complex for a new-comer like me. :)
>
> The difference between small and large files is the returned value by
> shouldUseMmap in /llvm/lib/Support/MemoryBuffer.cpp. For small files,
> shouldUseMmap returns false and the already created normal
> MemoryBuffer is kept and all is fine. But for larger files,
> shouldUseMmap returns true and the already created normal MemoryBuffer
> is destroyed and a new memory mapped one is created. This becomes the
> OwningPtr<MemoryBuffer> Code memorybuffer in ClangFormat.cpp.
> Now when the time comes to write the reformatted text in-place to the
> input file, llvm::raw_fd_ostream FileStream cannot be opened if the
> MemoryBuffer is a memory mapped one. Apparently Windows does not allow
> that. For small files, no Mmap buffer was created and all is fine.
>
> I do not know nearly enough about the file handling internals to
> propose a good fix.
> If shouldUseMmap is modified to always return false, clang-format
> works fine on large files. But that would be a very course "fix". I
> wanted to try releasing the Mmap interface to the file, right before
> writing back to it. But I was not able to figure out how to do that.
>
> Thanks for the help,
>      Johan
>
> [1]  http://llvm.org/bugs/show_bug.cgi?id=17216
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
>
>
> -- http://www.nuanti.com
> the browser experts
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20131103/81bfd230/attachment.html>