Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

(Blocks: 1 bug)

unspecified
mozilla34
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Comment hidden (empty)
(Assignee)

Updated

3 years ago
Assignee: nobody → evilpies
(Assignee)

Comment 1

3 years ago
Created attachment 8473760 [details] [diff] [review]
Remove unused messages

-47 messages!
Attachment #8473760 - Flags: review?(till)
Comment on attachment 8473760 [details] [diff] [review]
Remove unused messages

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

This is full of all kinds of win.

I think jorendorff's suggestion to group messages by area is also pretty good, but this should definitely go in as-is.
Attachment #8473760 - Flags: review?(till) → review+
(Assignee)

Comment 3

3 years ago
Yeah I plan on doing the grouping in this bug as well.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bec67212ac4
Great!
Status: NEW → ASSIGNED
Keywords: leave-open
OS: Linux → All
Hardware: x86_64 → All
(Assignee)

Comment 5

3 years ago
Created attachment 8474096 [details] [diff] [review]
reorder

These are most of the messages that we relatively easy to group with a script.
Attachment #8474096 - Flags: review?(till)
Comment on attachment 8474096 [details] [diff] [review]
reorder

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

Nice!

Please insert empty lines above the section comments, so they stick out a bit more.

There are some more groups that could be extracted. Ideas are:
- asm.js
- date- and intl
- regexp
- internal errors, such as JSMSG_INACTIVE, JSMSG_DEAD_OBJECT, or JSMSG_UNKNOWN_FORMAT

Obviously, this patch already improves things substantially, so no need to do additional work here, but those (and others, I'm sure) would be nice follow-ups.

::: js/src/vm/StructuredClone.cpp
@@ +1755,5 @@
>      AssertHeapIsIdle(cx);
>      CHECK_REQUEST(cx);
>  
>      if (version > JS_STRUCTURED_CLONE_VERSION) {
> +        JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_SC_BAD_CLONE_VERSION);

This function should probably be named something else now that the numbers are gone from js.msg. Then again, the whole error reporting should be overhauled, so it just might not matter too much.
Attachment #8474096 - Flags: review?(till) → review+
(Assignee)

Comment 7

3 years ago
Thanks for reviewing! It seems like most of the groups you suggested are included in that patch:
- asm.js
- intl
- regexp

Internal errors are an obvious next step. Object related errors might get their own subcategories. We also have some errors for non-JS APIs.
(In reply to Tom Schuster [:evilpie] from comment #7)
> Thanks for reviewing! It seems like most of the groups you suggested are
> included in that patch:

Oh, I see. I checked somewhat superficially, so didn't see those groups when I did the "are there more potential groups" pass.

> - asm.js

JSMSG_USE_ASM_DIRECTIVE_FAIL isn't in that group, which threw me off.

> - intl

Maybe rename to "Intl & Date" and move the remaining Date-related message (JSMSG_INVALID_DATE) there, too?

> - regexp

Here, the frontend message threw me off, but it makes sense where it is.

> 
> Internal errors are an obvious next step. Object related errors might get
> their own subcategories. We also have some errors for non-JS APIs.


Oh, and uber-nit: s/self hosting/self-hosting/
(Assignee)

Comment 9

3 years ago
Sorry going to fix that nit in the next patch.
https://hg.mozilla.org/integration/mozilla-inbound/rev/408f6612a5e9

It seems like there are some warnings that used to exists, but don't anymore:
- yield without a value in legacy generators
- accessing function.caller
(Assignee)

Comment 10

3 years ago
Created attachment 8474140 [details] [diff] [review]
next batch
Attachment #8474140 - Flags: review?(till)
https://hg.mozilla.org/mozilla-central/rev/8bec67212ac4
https://hg.mozilla.org/mozilla-central/rev/408f6612a5e9
Comment on attachment 8474140 [details] [diff] [review]
next batch

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

Very nice, it's really great that you're doing this!

::: js/src/builtin/Map.js
@@ +13,5 @@
>      /* Step 3-4. */
>      try {
>          callFunction(std_Map_has, M);
>      } catch (e) {
> +        ThrowError(JSMSG_INCOMPATIBLE_PROTO, "Map", "forEach", typeof M);

Hum, preexisting, but I'm not a fan of this way of ensuring that an object is a Map at all. Can you add a short comment explaining that std_Map_has will throw an exception for non-Map objects and we ignore that, but throw another exception here?

(Theoretically, we have better ways of detecting the slot, but they may not be fully baked, and changing this is way out of scope for this patch anyway.)

::: js/src/builtin/Set.js
@@ +13,5 @@
>      /* Step 3-4. */
>      try {
>          callFunction(std_Set_has, S);
>      } catch (e) {
> +        ThrowError(JSMSG_INCOMPATIBLE_PROTO, "Set", "forEach", typeof S);

Same as for Map#forEach: please add a short comment explaining this while you're here.

::: js/src/js.msg
@@ +144,5 @@
> +MSG_DEF(JSMSG_ACCESSOR_DEF_DENIED,     1, JSEXN_ERR, "Permission denied to define accessor property '{0}'")
> +MSG_DEF(JSMSG_DEAD_OBJECT,             0, JSEXN_TYPEERR, "can't access dead object")
> +MSG_DEF(JSMSG_UNWRAP_DENIED,           0, JSEXN_ERR, "permission denied to unwrap object")
> +
> +// JSAPI-only (Don't throw to JS)

Maybe "Not thrown as JS exceptions"?

@@ +399,5 @@
>  MSG_DEF(JSMSG_TOO_MANY_PARENS,         0, JSEXN_INTERNALERR, "too many parentheses in regular expression")
>  MSG_DEF(JSMSG_UNMATCHED_RIGHT_PAREN,   0, JSEXN_SYNTAXERR, "unmatched ) in regular expression")
>  MSG_DEF(JSMSG_UNTERM_CLASS,            0, JSEXN_SYNTAXERR, "unterminated character class")
>  
> +// Self-hosting

thanks :)
Attachment #8474140 - Flags: review?(till) → review+
(Assignee)

Comment 13

3 years ago
Almost done here, only 50 or so left to be sorted through.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f78b69406b6
Blocks: 1055301
https://hg.mozilla.org/mozilla-central/rev/6f78b69406b6
(Assignee)

Comment 15

3 years ago
I guess I won't to the rest here anymore.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Keywords: leave-open
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.