Provide a readline function for xpcshell to read a line from stdin

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
11 years ago
6 years ago

People

(Reporter: Fallen, Assigned: Fallen)

Tracking

({fixed1.9.1})

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Posted patch Add ReadLine to xpcshell - v1 (obsolete) β€” β€” Splinter Review
In bug 300841, readline capabilities were added to js shell. I'd like this to work for xpcshell too.

Since xpcshell already provides a GetLine function, I'm using that instead of the js_fgets from bug 300841's jsscan.c.

This patch doesn't address the problems from bug 105707, when a new patch (after 6 years!) for that bug is created, the line length issue needs to be tackled for this change too.

Comments welcome.
Attachment #322290 - Flags: superreview?(brendan)
Attachment #322290 - Flags: review?(brendan)
Comment on attachment 322290 [details] [diff] [review]
Add ReadLine to xpcshell - v1

Really want mrbkap to look at this. Also would like to unify js and xpc shells (there's an old bug asking for that), not diverge them further.

/be
Attachment #322290 - Flags: review?(brendan) → review?(mrbkap)
Comment on attachment 322290 [details] [diff] [review]
Add ReadLine to xpcshell - v1

>Index: js/src/xpconnect/shell/xpcshell.cpp
>@@ -173,16 +206,71 @@ my_ErrorReporter(JSContext *cx, const ch
> JS_STATIC_DLL_CALLBACK(JSBool)
>+ReadLine(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+{
>+    // While 4096 might be quite arbitrary, this is something to be fixed in
>+    // bug 105707. It is also the same limit as in ProcessFile.
>+    char buf[4096];
>+    char *promptBytes;

This isn't needed.

>+    /* If a prompt was specified, construct the string */

I'd rather see this written as:

if (argc > 0) {
    str = JS_ValueToString(cx, argv[0]);
    if (!str)
      return JS_FALSE;
    argv[0] = STRING_TO_JSVAL(str);
} else {
    str = JSVAL_TO_STRING(JS_GetEmptyStringValue(cx));
}

Note a couple of things:
- JS_ValueToString reports errors (such as OOM) so you don't have to.
- Generally, xpcshell.cpp doesn't overbrace one-line if conditions.
- Assigning the returned string back into argv[0] is a good habit to prevent GC hazards.

>+    rv = GetLine(cx, buf, gInFile, promptBytes);

Then this becomes:
    rv = GetLine(cx, buf, gInFile, JS_GetStringBytes(str));

>+    free(promptBytes);

And no need to free promptBytes.

>+    if (!rv) {
>+        return JS_FALSE;
>+    }

Overbracing here, I'm not going to point this out every time.

>+    /* Strip newline character */
>+    if (strlen(buf)) {
>+        buf[strlen(buf) - 1] = '\0';
>+    }

If you're going to call strlen multiple times, I'd rather see you stick it in a local variable to avoid counting the characters 3 times.

>+    /* Treat the empty string specially. */
>+    if (strlen(buf) == 0) {

This isn't a big deal if you stash the strlen beforehand, a better way to write this test would be:
    if (buf[0] == '\0')

I'm also not certain this is needed. It seems like JS_NewString* deals with 0-length strings fine. If this came from my ReadLine, the reason there was to avoid the shrinking realloc with 0 size.

>+     /* Turn buf into a JSString */
>+    str = JS_NewStringCopyZ(cx, buf);

Since you've stashed the string length above, this can be a JS_NewStringCopyN.
Attachment #322290 - Flags: superreview?(brendan)
Attachment #322290 - Flags: review?(mrbkap)
Attachment #322290 - Flags: review-
Blocks: 455752
Posted patch Add ReadLine to xpcshell - v2 (obsolete) β€” β€” Splinter Review
This patch takes all review comments into account. Sorry that this patch diverges xpcshell and jsshell more, but the changes required for bug 209176 are too massive for me to take care of now. That would require a lot more testing and possibly more knowledge about the works of jsshell/xpcshell and the js api.
Attachment #322290 - Attachment is obsolete: true
Attachment #339663 - Flags: review?
Attachment #339663 - Flags: review? → review?(mrbkap)
Attachment #339663 - Flags: review?(mrbkap) → review-
Comment on attachment 339663 [details] [diff] [review]
Add ReadLine to xpcshell - v2

A couple more nitpicks. Sorry for taking so long to get to this :-/

>+    /* Get a line from the infile */
>+    JSBool rv = GetLine(cx, buf, gInFile, JS_GetStringBytes(str));
>+
>+    if (!rv)
>+        return JS_FALSE;

No need for single-use 'rv' here. Just do:

if (!GetLine(cx, buf, gInFile, JS_GetStringBytes(str))
    return JS_FALSE;

>+    /* Strip newline character added by GetLine() */
>+    unsigned int buflen = strlen(buf);
>+    if (buf[0] != '\0')
>+        buf[buflen - 1] = '\0';

It looks like (because of the call to GetLine, buf[buflen - 1] might not be a newline. This should be:

if (buf[buflen - 1] == '\n')
    buf[--buflen] = '\0';

(notice the --buflen, to avoid copying the null character in the JS_NewStringCopyN).

I didn't look closely enough before. The second reason that the ReadLine in js.cpp handles the empty string specially is to indicate EOF to the script. As the patch is written currently, it's impossible to tell an empty line from EOF. So, after the call to strlen, it seems like we should have:

if (buflen == 0 && feof(gInFile)) {
    *rval = JSVAL_NULL;
    return JS_TRUE;
}

if (buflen > 0 && buf[buflen - 1] == '\n') {
    buf[--buflen] = '\0';
}
Updated to reflect comments. I've removed the '\0' termination since it's constructed passing the length anyway.
Attachment #345267 - Flags: review?(mrbkap)
Attachment #345267 - Flags: superreview+
Attachment #345267 - Flags: review?(mrbkap)
Attachment #345267 - Flags: review+
Comment on attachment 345267 [details] [diff] [review]
Add ReadLine to xpcshell - v3

Cool, thanks.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Attachment #345267 - Flags: approval1.9.1b2?
Comment on attachment 345267 [details] [diff] [review]
Add ReadLine to xpcshell - v3

We'll get to this after beta 2.
Attachment #345267 - Flags: approval1.9.1b2? → approval1.9.1b2-
Comment on attachment 345267 [details] [diff] [review]
Add ReadLine to xpcshell - v3

a191=beltzner
Attachment #345267 - Flags: approval1.9.1+
Posted patch Patch for checkin β€” β€” Splinter Review
This is v3 for checkin to 1.9.1 (includes checkin message and user)
Attachment #350371 - Flags: superreview+
Attachment #350371 - Flags: review+
Attachment #339663 - Attachment is obsolete: true
In case anyone comes here looking for what I backed out, I meant to type in bug 463824 as the bug number, not this one.
http://hg.mozilla.org/mozilla-central/rev/43900a377c77
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Keywords: fixed1.9.1
Depends on: 962208
You need to log in before you can comment on or make changes to this bug.