Closed Bug 1054322 Opened 5 years ago Closed 5 years ago

Cleanup js.msg


(Core :: JavaScript Engine, defect)

Not set





(Reporter: evilpie, Assigned: evilpie)




(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.
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]

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


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

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.

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.
Blocks: 1055301
I guess I won't to the rest here anymore.
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.