300841 - Provide a readline function to read a line from stdin

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Blake Kaplan (:mrbkap) (inactive)]

Apparently, the great language shootout requires a readline function. I've
written one which I'll attach.

Note that this function might be slightly unnecessary (The Great Language
Shootout is vague on requirements), since I notice that I can do:
echo "print('hello, world')" | js

and it prints "hello, world".

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: readline, v1

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 189360 [details] [diff] [review]
readline, v1

>+        size_t readlength = strlen(ret + retlength);
>+        retlength += readlength;

I've replaced these two lines with:
retlength += strlen(ret + retlength);

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

*** Bug 300835 has been marked as a duplicate of this bug. ***

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 189360 [details] [diff] [review]
readline, v1

> static JSBool
>+ReadLine(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+{
>+#define BUFSIZE 256
>+    /* function readline() */
>+    FILE *from = stdin;
>+    char *ret = NULL, *tmp;
>+    size_t retsize = BUFSIZE, retlength = 0;
>+    JSString *str;
>+
>+    ret = JS_malloc(cx, retsize);
>+    if (!ret)
>+        return JS_FALSE;

Hiding initialization in a forest of declarations hid the unnecessary ret =
NULL here.  I'd just move initializers down to nearest basic block leader that
dominates all uses.  But I think like a compiler too much.

The ret naming is not adding value, in my eyes, and it clashes with buf/BUF. 
Why not s/ret/buf/g?

>+    while (fgets(ret + retlength, retsize - retlength, from)) {

So fgets sucks (so does gets, fgets just sucks differently, and less).	It
doesn't return the line length, as jsscan.c's my_fgets does.  How about making
that a JS_FRIEND_API(int) js_fgets(...) export and using it here?

>+        size_t readlength = strlen(ret + retlength);

Then you can save a strlen.

[snip]

> static JSFunctionSpec shell_functions[] = {
>     {"version",         Version,        0},
>     {"options",         Options,        0},
>     {"load",            Load,           1},
>+    {"readline",        ReadLine,       0},
>     {"print",           Print,          0},
>     {"help",            Help,           0},
>     {"quit",            Quit,           0},
>     {"gc",              GC,             0},
>     {"trap",            Trap,           3},
>     {"untrap",          Untrap,         2},
>     {"line2pc",         LineToPC,       0},
>     {"pc2line",         PCToLine,       0},
> #ifdef DEBUG
>     {"dis",             Disassemble,    1},

D'oh, you forgot to update the parallel shell_help_messages array!

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: readline, v2

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 189387 [details] [diff] [review]
readline, v2

> static JSBool
>+ReadLine(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+{
>+#define BUFSIZE 256
>+    /* function readline() */

These comments seem to me more helpful before the entire function.  They might
say a bit more, too ;-).

>+    FILE *from;
>+    char *buf, *tmp;
>+    size_t bufsize, buflength;
>+    JSString *str;
>+
>+    from = stdin;
>+    buflength = 0;
>+    bufsize = BUFSIZE;
>+    buf = JS_malloc(cx, bufsize);
>+    if (!buf)
>+        return JS_FALSE;
>+
>+    while ((buflength += 
>+                js_fgets(buf + buflength, bufsize - buflength, from)) > 0) {

Need a separate int count or whatever to capture js_fgets's return value and
compare > 0, otherwise error or EOF without a \n just before EOF will loop
wrongly, infinitely in the latter case.

>+        /* Are we done? */
>+        if (buf[buflength - 1] == '\n') {
>+            buf[buflength - 1] = '\0';
>+            break;
>+        }
>+
>+        /* Else, grow our buffer for another pass. */
>+        tmp = JS_realloc(cx, buf, bufsize * 2);
>+        if (!tmp) {
>+            JS_free(cx, buf);
>+            return JS_FALSE;
>+        }
>+
>+        bufsize *= 2;
>+        buf = tmp;
>+    }
>+
>+    /* Shrink the buffer to the real size */
>+    tmp = JS_realloc(cx, buf, buflength);
>+    if (!tmp) {
>+        JS_free(cx, buf);
>+        return JS_FALSE;
>+    }

Oops, forgot buf = tmp after this paragraph (shrinking realloc may fail).

>+
>+    /* 
>+     * Turn buf into a JSString. Note that buflength includes the trailing null
>+     * character.
>+     */
>+    str = JS_NewString(cx, buf, buflength - 1);
>+    if (!str) {
>+        JS_free(cx, buf);
>+        return JS_FALSE;
>+    }
>+
>+    *rval = STRING_TO_JSVAL(str);
>+    return JS_TRUE;
>+}

Fix those and patch once more, I'll r+a=.

/be

Comment 7

[Brendan Eich [:brendan]]

> Oops, forgot buf = tmp after this paragraph (shrinking realloc may fail).

Tired, meant to write "may move", not "may fail".  Randell Jesup observed this
with the BSD size-classifying malloc.

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: readline, v3

All comments should addressed. Also fixed a problem dealing with the empty line
(it's too bad realloc can't assert on a zero-sized request!). I really need to
sit and look at my patches more carefully to avoid these sorts of silly
mistakes.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 189400 [details] [diff] [review]
readline, v3

Nits:

- gotlength instead of readlength?  Anyway, wrap so the js_fgets underhangs the
first letter of the variable name in the nested assignment in the while loop's
condition).

- Period at end of "Shrink the buffer to the real size".

Fix those and r+a=me.

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.

It occurs to me that xpcshell might want this function as well. Should I port
this to the xpcshell?

Comment 11

[Brendan Eich [:brendan]]

Sure, unless you are inclined to make a libjsshell that both js and xpcshell can
link against (might not be hard, given the descent of xpcshell.cpp from js.c
long ago, with mostly additive mutations ;-).  If you're not inclined to do that
just now, how about at least filing a bug on that?  Thanks,

/be

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

This is really fixed.

Comment 13

[Daniel Boelzle [:dbo]]

(In reply to comment #10)
> It occurs to me that xpcshell might want this function as well. Should I port
> this to the xpcshell?

I could need that, it's appreciated.