355827 - SpiderMonkey's readline() can't distinguish EOF

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[KATO Kazuyoshi]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.4) Gecko/20060613 Camino/1.0.2
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.4) Gecko/20060613 Camino/1.0.2

SpiderMonkey's readline() function returns empty string at the end of flie.
However we can't distinguish between "" and EOF.

I think readline() must returns null at the end of file.


Reproducible: Always

Steps to Reproduce:
% cat ~/a.js
for (var i = 0; i < 3; i++) {
   var ln = readline();
   print(typeof ln + "\t'" + ln + "'");
}
% echo | ./Darwin_DBG.OBJ/js ~/a.js
string  ''
string  ''
string  ''
%

Comment 1

[KATO Kazuyoshi]

Attachment: readline() returns null at EOF

Comment 2

[Brian Crowder]

+        if (feof(from))
+            *rval = JSVAL_NULL;
+        else
+            *rval = JS_GetEmptyStringValue(cx);

how about
*rval = (feof(from) ? JSVAL_NULL : JS_GetEmptyStringValue(cx));
(with house-style indentation and spacing, of course) instead?

Otherwise, good patch.  I'll be glad to land it if Brendan is happy with it, and there aren't concerns that it will break compatibility with things that depend on an "" at EOF for some reason.

Comment 3

[Brian Crowder]

cc:ing brendan on this in case r? doesn't show up in his mailbox or doesn't end up sorted properly.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Oops, yeah. file_readln (in jsfile.c) also had to do this.

Comment 5

[KATO Kazuyoshi]

Attachment: patch v2

(In reply to comment #2)
> *rval = (feof(from) ? JSVAL_NULL : JS_GetEmptyStringValue(cx));
> (with house-style indentation and spacing, of course) instead?
Thank you and I follow your advice.


(In reply to comment #4)
> Oops, yeah. file_readln (in jsfile.c) also had to do this.
I think jsfile.c don't have this problem.

% cat ~/file.js 
var file = new File('/dev/stdin');
for (var i = 0; i < 3; i++) {
   var ln = file.readln()
   print(typeof ln + "\t'" + ln + "'");
}
% echo | ./Darwin_DBG.OBJ/js ~/file.js
/Users/kzys/file.js:3: warning: File /dev/stdin is closed, will open it for reading, proceeding
string  ''
object  'null'
object  'null'

Comment 6

[Brian Crowder]

Comment on attachment 257018 [details] [diff] [review]
patch v2

-        *rval = JS_GetEmptyStringValue(cx);
+        *rval = (feof(from) ? JSVAL_NULL : JS_GetEmptyStringValue(cx));

You don't need the extra () around the ternary expression.  Mind giving it one more spin?

Thanks

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 257018 [details] [diff] [review]
patch v2

Sorry, I should have reviewed this sooner. Only comment is a nit-pick: no need to parenthesize the ?: expression on the right of assignment to *rval.

Brian, can you land the nit-picked version of this patch? Thanks,

/be

Comment 8

[Brian Crowder]

The extra parens was actually my fault, I realize, btw.  :)  Just didn't notice how weird it looked until I saw it inline.  And yeah, I'll be glad to land the nit-picked version.

Comment 9

[Brian Crowder]

Attachment: nitpicked version

This is the one I'm landing on the trunk

Comment 10

[Brian Crowder]

js.c: 3.134