Closed Bug 1054322 Opened 5 years ago Closed 5 years ago

Cleanup js.msg

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(3 files)

No description provided.
Assignee: nobody → evilpies
-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+
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
Attached patch reorderSplinter Review
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+
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/
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
Attached patch next batchSplinter Review
Attachment #8474140 - Flags: review?(till)
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+
Almost done here, only 50 or so left to be sorted through.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f78b69406b6
Blocks: 1055301
I guess I won't to the rest here anymore.
Status: ASSIGNED → RESOLVED
Closed: 5 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.