Closed Bug 355827 Opened 13 years ago Closed 13 years ago

SpiderMonkey's readline() can't distinguish EOF

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: kzys, Assigned: crowderbt)

Details

Attachments

(1 file, 2 obsolete files)

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  ''
%
Attached patch readline() returns null at EOF (obsolete) — Splinter Review
Attachment #241533 - Flags: review?(brendan)
+        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
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.
Attached patch patch v2 (obsolete) — Splinter Review
(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)
Attachment #257018 - Flags: review?(brendan) → review?(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
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+
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.
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)
js.c: 3.134
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.