FileHandle.readall() broken when file already read from. #930

Closed
Benabik opened this Issue Feb 1, 2013 · 4 comments

Comments

Projects
None yet
2 participants
@Benabik
Member

Benabik commented Feb 1, 2013

Reported on #perl6 with example: https://gist.github.com/4687526

Simple winxed version (filename: iobug.winxed)

$load 'Test/More.pbc';
function main() {
    Test.More.plan(1);

    var file = open('iobug.winxed');
    file.readline();
    string rest = file.readall();
    Test.More.is(-1, indexof(rest, "\0"));
}

It appears that Parrot_io_readall_s (src/io/api.c:750) allocates a string the size of the entire file and then reads into that.

@Benabik Benabik closed this in 1d6430d Feb 1, 2013

@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban Feb 1, 2013

Member

If you change the behaviour of readall() to readrest() please also update the documentation at least.
Either seek at readall the filepointer to 0 (readall) or adjust the allocation (readrest).
Personally I would expect from IO.readall to read the whole file, not the rest.

Member

rurban commented Feb 1, 2013

If you change the behaviour of readall() to readrest() please also update the documentation at least.
Either seek at readall the filepointer to 0 (readall) or adjust the allocation (readrest).
Personally I would expect from IO.readall to read the whole file, not the rest.

@rurban rurban reopened this Feb 1, 2013

@Benabik

This comment has been minimized.

Show comment
Hide comment
@Benabik

Benabik Feb 1, 2013

Member

I'm not changing the behavior. I changed the correctness.

Member

Benabik commented Feb 1, 2013

I'm not changing the behavior. I changed the correctness.

@Benabik

This comment has been minimized.

Show comment
Hide comment
@Benabik

Benabik Feb 2, 2013

Member

The readall documentation does not say it rewinds the file if its already open. I see no reason why it should, and others don't appear to think it should (given that the bug report was based on using it to read the rest of the file). The only change requested, and performed, was to ensure that it didn't allocate more space in the string than it would actually read.

If you think it should be changed to rewind the file before reading, that should be a new ticket.

Member

Benabik commented Feb 2, 2013

The readall documentation does not say it rewinds the file if its already open. I see no reason why it should, and others don't appear to think it should (given that the bug report was based on using it to read the rest of the file). The only change requested, and performed, was to ensure that it didn't allocate more space in the string than it would actually read.

If you think it should be changed to rewind the file before reading, that should be a new ticket.

@Benabik Benabik closed this Feb 2, 2013

rurban pushed a commit that referenced this issue Jul 16, 2014

Reini Urban
[docs] Updated StringHandle.readall and FileHandle.readall docs
They read just the rest of the buffer if tell > 0. [GH #1084]
This is not what you would expect, but perl6 wanted it this way.
See GH #930 for the discussion.
@rurban

This comment has been minimized.

Show comment
Hide comment
@rurban

rurban Jul 16, 2014

Member

Ok, added a new ticket GH #1084 to document the unexpected readrest behavior. See branch rurban/readall-gh1084

Member

rurban commented Jul 16, 2014

Ok, added a new ticket GH #1084 to document the unexpected readrest behavior. See branch rurban/readall-gh1084

rurban pushed a commit that referenced this issue Jul 21, 2014

Reini Urban
[docs] Updated StringHandle.readall and FileHandle.readall docs
They read just the rest of the buffer if tell > 0. [GH #1084]
This is not what you would expect, but perl6 wanted it this way.
See GH #930 for the discussion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment