<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Sun, Nov 3, 2013 at 2:19 PM, Johan Engelen <span dir="ltr"><<a href="mailto:jbc.engelen@swissonline.ch" target="_blank" class="cremed">jbc.engelen@swissonline.ch</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  
    
  
  <div bgcolor="#FFFFFF" text="#000000"><div class="im">
    On 2-11-2013 23:16, Alp Toker wrote:<br>
    <blockquote type="cite">
      
      On 02/11/2013 20:48, Johan Engelen wrote:<br>
      <blockquote type="cite">Hi

        Alp, <br>
          The patch solves the problem, thanks! <br>
      </blockquote>
      <br>
      Glad to hear it!<br>
      <br>
      <blockquote type="cite">
        <br>
        One comment: the format function returns 'false' when no errors
        occurred. So I believe it should read <br>
              if (Rewrite.overwriteChangedFiles()) { <br>
                return false; <br>
              } <br>
      </blockquote>
      <br>
      Disagree. My patch is fine :-)<br>
      <br>
      It's just that the return value of overwriteChangedFiles() was
      mis-documented until my fix last week (r193594) so that might have
      caused confusion.<br>
      <br>
      But I'll appreciate if you can confirm:<br>
      <code><br>
      </code><code>   /// overwriteChangedFiles - Save all changed files
        to disk.</code><code><br>
      </code><code>   ///</code><code><br>
      </code><code>   /// Returns true if any files were not saved
        successfully.</code><code><br>
      </code><code>   /// Outputs diagnostics via the source manager's
        diagnostic engine</code><code><br>
      </code><code>   /// in case of an error.</code><code><br>
      </code><code>   bool overwriteChangedFiles();</code><code><br>
      </code><code> </code><code><br>
      </code><code>// Returns true on error.</code><code><br>
      </code><code> static bool format(std::string FileName) {</code><code><br>
      </code><code> ...</code><code><br>
      </code><code>       if (Rewrite.overwriteChangedFiles())</code><code><br>
      </code><code>         return true;</code><code><br>
      </code><code> ...</code><code><br>
      </code><code> }</code><br>
    </blockquote>
    <br></div>
    Agreed :-)<div class="im"><br>
    <br>
    <blockquote type="cite"> <br>
      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.<br>
    </blockquote>
    <br></div>
    (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... :-)<br>
    Adding a test case means adding to /llvm/tools/clang/test/Format ?<br>
    I'll see if I can figure it out from the other tests in the
    codebase.</div></blockquote><div><br></div><div>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.</div>
<div><br></div><div>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 ..</div>
<div><br></div><div>Cheers,<br>Daniel</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><span class="HOEnZb"><font color="#888888">
    -Johan</font></span><div><div class="h5"><br>
    <br>
    <br>
    <blockquote type="cite"> Alp.<br>
      <br>
      <br>
      <br>
      <blockquote type="cite">
        <br>
        Cheers, <br>
          Johan <br>
        <br>
        <br>
        On 2-11-2013 21:05, Alp Toker wrote: <br>
        <blockquote type="cite">Hello Johan, <br>
          <br>
          I've attached a patch I'm using locally to reduce flicker in
          my IDE <br>
          while running clang-format using atomic file save. <br>
          <br>
          I suspect it resolves you issue too, could you try it and
          report back? <br>
          <br>
          Alp. <br>
          <br>
          <br>
          On 01/11/2013 21:12, Johan Engelen wrote: <br>
          <blockquote type="cite">Hello all, <br>
               I've been looking into why clang-format cannot write
            in-place for <br>
            large files (> 16 kB) on Windows [1]. <br>
            I've found the culprit, but need help actually fixing it;
            the file <br>
            handling code is somewhat complex for a new-comer like me.
            :) <br>
            <br>
            The difference between small and large files is the returned
            value by <br>
            shouldUseMmap in /llvm/lib/Support/MemoryBuffer.cpp. For
            small files, <br>
            shouldUseMmap returns false and the already created normal <br>
            MemoryBuffer is kept and all is fine. But for larger files,
            <br>
            shouldUseMmap returns true and the already created normal
            MemoryBuffer <br>
            is destroyed and a new memory mapped one is created. This
            becomes the <br>
            OwningPtr<MemoryBuffer> Code memorybuffer in
            ClangFormat.cpp. <br>
            Now when the time comes to write the reformatted text
            in-place to the <br>
            input file, llvm::raw_fd_ostream FileStream cannot be opened
            if the <br>
            MemoryBuffer is a memory mapped one. Apparently Windows does
            not allow <br>
            that. For small files, no Mmap buffer was created and all is
            fine. <br>
            <br>
            I do not know nearly enough about the file handling
            internals to <br>
            propose a good fix. <br>
            If shouldUseMmap is modified to always return false,
            clang-format <br>
            works fine on large files. But that would be a very course
            "fix". I <br>
            wanted to try releasing the Mmap interface to the file,
            right before <br>
            writing back to it. But I was not able to figure out how to
            do that. <br>
            <br>
            Thanks for the help, <br>
                 Johan <br>
            <br>
            [1]  <a href="http://llvm.org/bugs/show_bug.cgi?id=17216" target="_blank" class="cremed">http://llvm.org/bugs/show_bug.cgi?id=17216</a>
            <br>
            _______________________________________________ <br>
            cfe-dev mailing list <br>
            <a href="mailto:cfe-dev@cs.uiuc.edu" target="_blank" class="cremed">cfe-dev@cs.uiuc.edu</a>
            <br>
            <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev" target="_blank" class="cremed">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev</a>
            <br>
          </blockquote>
        </blockquote>
        <br>
      </blockquote>
      <br>
      <pre cols="72">-- 
<a href="http://www.nuanti.com" target="_blank" class="cremed">http://www.nuanti.com</a>
the browser experts
</pre>
    </blockquote>
    <br>
  </div></div></div>

</blockquote></div><br></div></div>