SpiderMonkey's readline() can't distinguish EOF

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: KATO Kazuyoshi, Assigned: Brian Crowder)

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

11 years ago
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  ''
%
(Reporter)

Comment 1

11 years ago
Created attachment 241533 [details] [diff] [review]
readline() returns null at EOF
(Reporter)

Updated

11 years ago
Attachment #241533 - Flags: review?(brendan)
(Assignee)

Comment 2

11 years ago
+        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.
Assignee: general → crowder
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 3

11 years ago
cc:ing brendan on this in case r? doesn't show up in his mailbox or doesn't end up sorted properly.
Status: NEW → ASSIGNED
Oops, yeah. file_readln (in jsfile.c) also had to do this.
(Reporter)

Comment 5

11 years ago
Created attachment 257018 [details] [diff] [review]
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'
Attachment #241533 - Attachment is obsolete: true
Attachment #257018 - Flags: review?(brendan)
Attachment #241533 - Flags: review?(brendan)
(Reporter)

Updated

11 years ago
Attachment #257018 - Flags: review?(brendan) → review?(crowder)
(Assignee)

Comment 6

11 years ago
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
Attachment #257018 - Flags: review?(crowder) → review-
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
Attachment #257018 - Flags: review?(crowder)
Attachment #257018 - Flags: review-
Attachment #257018 - Flags: review+
(Assignee)

Comment 8

11 years ago
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.
(Assignee)

Comment 9

11 years ago
Created attachment 258582 [details] [diff] [review]
nitpicked version

This is the one I'm landing on the trunk
Attachment #257018 - Attachment is obsolete: true
Attachment #258582 - Flags: review+
Attachment #257018 - Flags: review?(crowder)
(Assignee)

Comment 10

11 years ago
js.c: 3.134
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.