1504893 - Add a jsapi-test for omitting a line of context when it would be empty

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Jeff Walden [:Waldo]]

Empty line of context is going to be a Thing when there are Unicode errors in the rest of the line.  An error immediately at the point of the error, means there's no window after the current offset.  A line break immediately before the error, also means there's no window.

When there's an empty line of context, we decline to add a line of context to the error report.  This is something I did roughly in passing awhile back, but with encoding errors, it's probably a little more prominent.  Add some tests for it.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Implement JS::CompileDontInflate that takes JS::SourceText<Utf8Unit>& paralleling JS::Compile that takes JS::SourceText<char16_t>&

I really wanted JS::Compile for this, so template-y algorithms could call JS::Compile and not think about the type of SourceText passed in.  :-(  But I don't think we can eschew the DontInflate suffix just yet.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Add a jsapi-test for omitting a line of context when it would be empty

Comment 3

[Tooru Fujisawa [:arai]]

Comment on attachment 9022798 [details] [diff] [review]
Add a jsapi-test for omitting a line of context when it would be empty

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

::: js/src/jsapi-tests/testEmptyWindowIsOmitted.cpp
@@ +133,5 @@
> +    CHECK(!compile(chars, N - 1, &script));
> +
> +    JS::RootedValue exn(cx);
> +    CHECK(JS_GetPendingException(cx, &exn));
> +    JS_ClearPendingException(cx);

I wonder if it's better moving js::GetAndClearException to more accessible place (now it's in vm/Interpreter.h, but apparently usage is already not vm nor interpreter specific)

@@ +165,5 @@
> +        ++iter;
> +        CHECK(iter == notes->end());
> +    } else {
> +        CHECK(sizeof(CharT) == 2);
> +        CHECK(!badCodeUnits);

can you put some comment that UTF-16 compilation isn't supposed to add notes to error at this point?
(to help someone hit this assertion in case it changes in the future)

Comment 4

[Tooru Fujisawa [:arai]]

Comment on attachment 9022798 [details] [diff] [review]
Add a jsapi-test for omitting a line of context when it would be empty

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

::: js/src/jsapi-tests/testEmptyWindowIsOmitted.cpp
@@ +113,5 @@
> +bool
> +compile(const CharT* chars, size_t len, JS::MutableHandleScript script)
> +{
> +    JS::RealmOptions globalOptions;
> +    JS::RootedObject global(cx, JS_NewGlobalObject(cx, getGlobalClass(), nullptr,

what's the reason why you're manually creating another global here?
JSAPITest has global which is already created in JSAPITest::init(),
and also there's createGlobal method even if you need to create new one

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0252a8bad879
Implement JS::CompileDontInflate that takes JS::SourceText<Utf8Unit>& paralleling JS::Compile that takes JS::SourceText<char16_t>&.  r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/97dadee2ab7c
Add a jsapi-test for omitting a line of context when it would be empty.  r=arai

Comment 6

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/0252a8bad879
https://hg.mozilla.org/mozilla-central/rev/97dadee2ab7c