On Tue, 30 Mar 2010, Levente Uzonyi wrote:

> On Mon, 29 Mar 2010, Andreas Raab wrote:
>
>> Hi Levente -
>> 
>> Could you check the failure of FileStreamTest>>testNextChunkOutOfBounds? 
>> This looks like it might be a problem with caching file stream behavior. If 
>> it's not (i.e., the infinite recursion is the 'expected' behavior) can we 
>> add something that addresses the issue?
>
> Seems like #primAtEnd: doesn't answer true if position is out of bounds and 
> there are no more bytes to read:

I realized that even if I push the changes for #basicUpTo: and friends 
there may be arbitrary code that assumes that #atEnd will return true if 
there's nothing more to read. How should this issue be fixed?

One solution is to change StandardFileStream >> #atEnd to something like 
this:

atEnd
 	"Answer whether the receiver is at its end.  "

 	collection ifNotNil: [
 		position < readLimit ifTrue: [ ^false ] ].
 	^(self primSizeNoError: fileID)
 		ifNil: [ true ]
 		ifNotNil: [ :size | size <= self position ]

But this means that #primAtEnd: is not used at all, so it's just a 
workaroud.

The other option is to change the VM code. This means changing == to >= 
like this:

For unix and mac (cross):

sqInt sqFileAtEnd(SQFile *f) {
 	/* Return true if the file's read/write head is at the end of the file. */

 	if (!sqFileValid(f)) return interpreterProxy->success(false);
 	return ftell(getFile(f)) >= getSize(f);
}

For windows:

sqInt sqFileAtEnd(SQFile *f) {
   win32FileOffset ofs;
   /* Return true if the file's read/write head is at the end of the file. */
   if (!sqFileValid(f)) FAIL();
   ofs.offset = 0;
   ofs.dwLow = SetFilePointer(FILE_HANDLE(f), 0, &ofs.dwHigh, FILE_CURRENT);
   return ofs.offset >= sqFileSize(f);
}

What do you think?


Levente

>
> FileStream forceNewFileNamed: 'testFileStreamAtEnd' do: [ :file |
> 	file position: 1000.
> 	self assert: file next isNil.
> 	self assert: file atEnd ].
>
> Read buffering doesn't affect this behavior:
>
> FileStream forceNewFileNamed: 'testFileStreamAtEnd' do: [ :file |
> 	file disableReadBuffering.
>        file position: 1000.
>        self assert: file next isNil.
>        self assert: file atEnd ].
>
> Really:
>
> FileStream forceNewFileNamed: 'testFileStreamAtEnd' do: [ :file |
> 	| fileID buffer1 count |
> 	file disableReadBuffering.
> 	file position: 1000.
> 	fileID := file instVarNamed: #fileID.
> 	buffer1 := String new: 1.
> 	count := file primRead: fileID into: buffer1 startingAt: 1 count: 1.
> 	self assert: count = 0.
> 	self assert: (file primAtEnd: fileID) ].
>
> We can work around this issue with StandardFileStream >> #upTo: and friends 
> (by replacing the old code which uses recursion and is pretty inefficient 
> btw), but I think #atEnd should answer true in this case.
>
>
> Levente
>
>> 
>> Thanks,
>>  - Andreas
>> 
>> 
>
>