Send non-JS output (e.g. errors) to stderr, instead of stdout. (issue 11365202)

[dub...@chromium.org]

Reviewers: Yang,

Message:
Please take a look.

Description:
Send non-JS output (e.g. errors) to stderr, instead of stdout.

The Chromium build uses this executable and needs to be able to
separate errors from JS output.

Please review this at https://codereview.chromium.org/11365202/

SVN Base: http://v8.googlecode.com/svn/branches/bleeding_edge/

Affected files:
M samples/shell.cc


Index: samples/shell.cc
===================================================================
--- samples/shell.cc (revision 12939)
+++ samples/shell.cc (working copy)
@@ -72,7 +72,7 @@
v8::HandleScope handle_scope;
v8::Persistent<v8::Context> context = CreateShellContext();
if (context.IsEmpty()) {
- printf("Error creating context\n");
+ fprintf(stderr, "Error creating context\n");
return 1;
}
context->Enter();
@@ -226,7 +226,8 @@
// alone JavaScript engines.
continue;
} else if (strncmp(str, "--", 2) == 0) {
- printf("Warning: unknown flag %s.\nTry --help for options\n", str);
+ fprintf(stderr,
+ "Warning: unknown flag %s.\nTry --help for options\n", str);
} else if (strcmp(str, "-e") == 0 && i + 1 < argc) {
// Execute argument given to -e option directly.
v8::Handle<v8::String> file_name = v8::String::New("unnamed");
@@ -237,7 +238,7 @@
v8::Handle<v8::String> file_name = v8::String::New(str);
v8::Handle<v8::String> source = ReadFile(str);
if (source.IsEmpty()) {
- printf("Error reading '%s'\n", str);
+ fprintf(stderr, "Error reading '%s'\n", str);
continue;
}
if (!ExecuteString(source, file_name, false, true)) return 1;
@@ -249,20 +250,20 @@

// The read-eval-execute loop of the shell.
void RunShell(v8::Handle<v8::Context> context) {
- printf("V8 version %s [sample shell]\n", v8::V8::GetVersion());
+ fprintf(stderr, "V8 version %s [sample shell]\n", v8::V8::GetVersion());
static const int kBufferSize = 256;
// Enter the execution environment before evaluating any code.
v8::Context::Scope context_scope(context);
v8::Local<v8::String> name(v8::String::New("(shell)"));
while (true) {
char buffer[kBufferSize];
- printf("> ");
+ fprintf(stderr, "> ");
char* str = fgets(buffer, kBufferSize, stdin);
if (str == NULL) break;
v8::HandleScope handle_scope;
ExecuteString(v8::String::New(str), name, true, true);
}
- printf("\n");
+ fprintf(stderr, "\n");
}


@@ -310,31 +311,31 @@
if (message.IsEmpty()) {
// V8 didn't provide any extra information about this error; just
// print the exception.
- printf("%s\n", exception_string);
+ fprintf(stderr, "%s\n", exception_string);
} else {
// Print (filename):(line number): (message).
v8::String::Utf8Value filename(message->GetScriptResourceName());
const char* filename_string = ToCString(filename);
int linenum = message->GetLineNumber();
- printf("%s:%i: %s\n", filename_string, linenum, exception_string);
+ fprintf(stderr, "%s:%i: %s\n", filename_string, linenum,
exception_string);
// Print line of source code.
v8::String::Utf8Value sourceline(message->GetSourceLine());
const char* sourceline_string = ToCString(sourceline);
- printf("%s\n", sourceline_string);
+ fprintf(stderr, "%s\n", sourceline_string);
// Print wavy underline (GetUnderline is deprecated).
int start = message->GetStartColumn();
for (int i = 0; i < start; i++) {
- printf(" ");
+ fprintf(stderr, " ");
}
int end = message->GetEndColumn();
for (int i = start; i < end; i++) {
- printf("^");
+ fprintf(stderr, "^");
}
- printf("\n");
+ fprintf(stderr, "\n");
v8::String::Utf8Value stack_trace(try_catch->StackTrace());
if (stack_trace.length() > 0) {
const char* stack_trace_string = ToCString(stack_trace);
- printf("%s\n", stack_trace_string);
+ fprintf(stderr, "%s\n", stack_trace_string);
}
}
}

[sven...@chromium.org]

Hmmm, I thought that Chromium uses d8 now and shell.cc is now what it was
actually meant to be: A trivial example how to embed v8. I don't think we
have
any test coverage for shell anymore.

https://codereview.chromium.org/11365202/

[yan...@chromium.org]

Some comments.


https://codereview.chromium.org/11365202/diff/1/samples/shell.cc
File samples/shell.cc (right):

https://codereview.chromium.org/11365202/diff/1/samples/shell.cc#newcode253
samples/shell.cc:253: fprintf(stderr, "V8 version %s [sample shell]\n",
v8::V8::GetVersion());
This is the prompt of the interactive shell and must not go to stderr.

https://codereview.chromium.org/11365202/diff/1/samples/shell.cc#newcode260
samples/shell.cc:260: fprintf(stderr, "> ");
Ditto.

https://codereview.chromium.org/11365202/diff/1/samples/shell.cc#newcode266
samples/shell.cc:266: fprintf(stderr, "\n");
Ditto.

https://codereview.chromium.org/11365202/diff/1/samples/shell.cc#newcode314
samples/shell.cc:314: fprintf(stderr, "%s\n", exception_string);
This reports uncaught exceptions caused in javascript. I think the
policy should be that error messages caused in this shell (like failed
read of a file) should go to stderr and error messages of the javascript
runtime should go to stdout. Same goes for all the output in this
function.

https://codereview.chromium.org/11365202/

[dub...@chromium.org]

On 2012/11/13 08:02:04, Sven Panne wrote:
> Hmmm, I thought that Chromium uses d8 now and shell.cc is now what it was
> actually meant to be: A trivial example how to embed v8. I don't think we
> have
> any test coverage for shell anymore.

out/Debug/v8_shell in Chromium is built from shell.cc.

https://codereview.chromium.org/11365202/

[dub...@chromium.org]

https://codereview.chromium.org/11365202/diff/1/samples/shell.cc
File samples/shell.cc (right):

https://codereview.chromium.org/11365202/diff/1/samples/shell.cc#newcode253
samples/shell.cc:253: fprintf(stderr, "V8 version %s [sample shell]\n",
v8::V8::GetVersion());

On 2012/11/13 08:48:17, Yang wrote:
> This is the prompt of the interactive shell and must not go to stderr.

I thought I mentioned this yesterday, but that is the way that Python
does it.

~/dev/chromium/src$ python > /tmp/python-output
Python 2.7.3 (default, Aug 1 2012, 05:14:39)
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more
information.
>>> def a():
... print 3
...
>>> a()
>>>
~/dev/chromium/src$ cat /tmp/python-output
3

It seems reasonable to me, but I can change that if you'd like.

https://codereview.chromium.org/11365202/diff/1/samples/shell.cc#newcode314
samples/shell.cc:314: fprintf(stderr, "%s\n", exception_string);

On 2012/11/13 08:48:17, Yang wrote:
> This reports uncaught exceptions caused in javascript. I think the
policy should
> be that error messages caused in this shell (like failed read of a
file) should
> go to stderr and error messages of the javascript runtime should go to
stdout.
> Same goes for all the output in this function.

I disagree. An uncaught exception needs to be logged to stderr, not
stdout.

In the Chromium build use case, we have a script that generates a C++
class by printing it to stdout, so we capture stdout from the script and
send it to a file. If uncaught exceptions aren't printed to stderr, we
have no way of actually extracting the error message.

Python and Ruby also send messages from uncaught exceptions to stderr:

~/dev/chromium/src$ python -c "raise Exception" > /dev/null
Traceback (most recent call last):
File "<string>", line 1, in <module>
Exception

~/dev/chromium/src$ ruby -e "raise 'blah'" > /dev/null
-e:1: blah (RuntimeError)

https://codereview.chromium.org/11365202/

[yan...@chromium.org]

I see. LGTM then. I'll land this for you.

https://codereview.chromium.org/11365202/