<div dir="rtl"><div dir="ltr">Hi Kim,</div><div dir="ltr"><br></div><div dir="ltr">I think the code you quoted above</div><div dir="ltr"><br></div><div dir="ltr"><div dir="ltr">    std::error_code EC;</div><div dir="ltr">    std::unique_ptr<MemoryBuffer> Result(</div><div dir="ltr">        new (NamedBufferAlloc(Filename))</div><div dir="ltr">        MemoryBufferMMapFile(RequiresNullTerminator, FD, MapSize, Offset, EC));</div><div dir="ltr">    if (!EC)</div><div dir="ltr">      return std::move(Result);</div><div><br></div><div>shows EC is an output variable: it is constructed before MemoryBufferMMapFile() and tested just after. The missing & is probably a typo.</div><div><br></div><div>For a unit test your second approach seems better since it directly tests MemoryBufferMMapFile: pass MemoryBufferMMapFile invalid FD or MapSize and test for errors.</div><div><br></div><div>Interesting this wen unnoticed. MemoryBufferMMapFile probably gets valid arguments and never fails.</div><div><br></div><div>Yaron</div><div><br></div></div></div><div class="gmail_extra"><br><div class="gmail_quote"><div dir="ltr">2014-12-11 7:21 GMT+02:00 Kim Gräsman <span dir="ltr"><<a href="mailto:kim.grasman@gmail.com" target="_blank">kim.grasman@gmail.com</a>></span>:</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Yaron,<br>
<span class=""><br>
On Wed, Dec 10, 2014 at 11:34 PM, Yaron Keren <<a href="mailto:yaron.keren@gmail.com">yaron.keren@gmail.com</a>> wrote:<br>
> Before this bug report gets lost, Kim is correct about the error code EC,<br>
> it's an output argument for MemoryBufferMMapFile::MemoryBufferMMapFile and<br>
> must be passed by reference, like:<br>
><br>
><br>
> Index: lib/Support/MemoryBuffer.cpp<br>
> ===================================================================<br>
> --- lib/Support/MemoryBuffer.cpp        (revision 223970)<br>
> +++ lib/Support/MemoryBuffer.cpp        (working copy)<br>
> @@ -203,7 +203,7 @@<br>
><br>
>  public:<br>
>    MemoryBufferMMapFile(bool RequiresNullTerminator, int FD, uint64_t Len,<br>
> -                       uint64_t Offset, std::error_code EC)<br>
> +                       uint64_t Offset, std::error_code &EC)<br>
>        : MFR(FD, false, sys::fs::mapped_file_region::readonly,<br>
>              getLegalMapSize(Len, Offset), getLegalMapOffset(Offset), EC) {<br>
>      if (!EC) {<br>
><br>
> This passes regression tests.<br>
<br>
</span>I started trying to create a unit test in<br>
llvm/unittests/Support/MemoryBufferTest.cpp yesterday that proves this<br>
change, but I think I started at the wrong level. I went at trying to<br>
prove that MemoryBuffer::getOpenFile returned an invalid object on<br>
error, but that would mean having to force mapping to fail<br>
(consistently on all platforms).<br>
<br>
Maybe it's better to add a test that MemoryBufferMMapFile's<br>
constructor returns the error code through an out arg? Then It should<br>
be trivial to test with an invalid FD or something.<br>
<br>
Let me know if you have other ideas, I can try and put a patch together today.<br>
<span class="HOEnZb"><font color="#888888"><br>
- Kim<br>
</font></span></blockquote></div><br></div>