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

[Johan Engelen]

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.

-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/03c2b475/attachment.html>