1426455 - Add a shell builtin to read all input from stdin as a Uint8Array

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, enhancement, P5)

Description

[Jason Orendorff [:jorendorff]]

Currently there's just readline(). Reading lines of text and reading bytes are really two different things though.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: readStdin shell builtin function

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8938084 [details] [diff] [review]
readStdin shell builtin function

Review of attachment 8938084 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/shell/OSObject.cpp
@@ +824,5 @@
> +
> +    // Read stdin to EOF or error.
> +    Vector<uint8_t> bytes(cx);
> +    int c;
> +    while ((c = getchar()) != EOF) {

Feel free to disagree, but I think mentioning stdin here would be more clear, since you do it below with ferror(). So getc(stdin) or fgetc(stdin).

@@ +828,5 @@
> +    while ((c = getchar()) != EOF) {
> +        MOZ_ASSERT((c & ~0xff) == 0);
> +        if (!bytes.append(uint8_t(c)))
> +            return false;
> +    }

I hope the optimizer does a good job with this character-at-a-time loop! Hm... can it? As in, does this *have* to do a system call per character when reading from stdin? Oh well, I'm not going to worry about it.

@@ +841,5 @@
> +    if (!obj)
> +        return false;
> +    {
> +        JS::AutoAssertNoGC nogc;
> +        bool isShared_;

Why the underscore? I thought that was usually used for class members, not temporaries.

@@ +983,5 @@
>  "  exception on failure."),
>  
> +    JS_FN_HELP("readStdin", os_readStdin, 0, 0,
> +"readStdin()",
> +"  Reads bytes from stdin until reaching EOF."),

Hm, this seems unfortunate.

  os.file.readFile("filename") returns a string. 

  os.file.readFile("filename", "binary") returns a typed array.

  os.file.readStdin() returns a string.

Admittedly, the whole binary argument to readFile is a retrofitted hack and probably should be an options object anyway. But the various return values are already surprising, and it seems sad to make them more so. How would you feel about either (1) making readStdin() return a string, and readStdin("binary") return a typed aray; or moving everything to

  os.file.readFile() - returns typed array
  os.file.readFileAsString() - returns string
  os.file.readStdin() - returns typed array (as you have it now)

It would require fixing up some tests, but I doubt we use readFile all that much. (Note that snarf() and read() will need to alias os.file.readFileAsString.) From my grepping, I see zero tests that would be affected, only devtools/rootAnalysis/*.js.

I dunno, it's still a little weird, and perhaps more effort than you want to put into this. What do you think?

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8938084 [details] [diff] [review]
readStdin shell builtin function

Clearing review for now in case you're still waiting on me. Feel free to argue.

Comment 4

[Nicolas B. Pierron [:nbp]]

Jason, is this blocking any other bug which might depend on it? Feel free to raise the priority if needed.

Comment 5

[Jason Orendorff [:jorendorff]]

P5 is right. I'm not totally sure I care about this feature anymore? I guess we might as well land it, will keep ni?me.