[PATCH] run the unit test for MemoryBuffer::getOpenFile in two different modes

[Eli Bendersky]

On Tue, Jul 23, 2013 at 7:12 AM, Rafael Espíndola <
rafael.espindola at gmail.com> wrote:

> LGTM, but please also take a look at my (late, sorry) comments on the
> original patch.
>
>
Thanks for the review. Here are your original comments:

>Unless you want to test unbuffered files too, it might be better to do
>something like
>{
> raw_fd_ostream OF(TestFD, true, true);
>....
>}
>OwningBuffer Buf;

I don't mind about buffering - really just wanted the results to be seen in
the file immediately without closing it. The new patch explicitly separates
the two cases: (1) file is still open and (2) file is close and then
reopened for reading.

>You probably want to pass the buff size, not the filesize down to
>MemoryBuffer::getOpenFile.

You are probably right; I was following the usage of getOpenFile within
gold-plugin.cpp, which deals with "files within files" and also passes the
size of the sub-file into FileSize. Now looking more closely on that
invocation, something doesn't make sense: When MapSize = -1, getOpenFile
sets MapSize = FileSize. But then, shouldUseMmap asserts:

  size_t End = Offset + MapSize;
  assert(End <= FileSize);

Meaning that if a positive offset is passed, and MapSize = 1 this assertion
would fire (unless shouldUseMmap bails out for small sizes).
So is gold-plugin.cpp using the API incorrectly? Note that actually using
this Offset is uncommon, ISTM only gold/lto related code uses it.

[in any case it would probably be beneficial to explain the API in more
detail in a comment - I can do that with my next commit there]

Eli






>  On 22 July 2013 19:32, Eli Bendersky <eliben at google.com> wrote:
> > This patch refactors the recently added unit test for
> > MemoryBuffer::getOpenFile to run in two different modes: with and without
> > reopening the temporary file between creating it and mapping it with
> > MemoryBuffer.
> >
> > PTAL,
> > Eli
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130723/f96bf137/attachment.html>