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

[Alp Toker]

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;||
||...||
||}|

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.


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/20131102/fd2cc6ec/attachment.html>