Closed
Bug 1054322
Opened 10 years ago
Closed 10 years ago
Cleanup js.msg
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: evilpie, Assigned: evilpie)
References
Details
Attachments
(3 files)
31.25 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
59.60 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
26.38 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → evilpies
Comment 2•10 years ago
|
||
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•10 years ago
|
||
Yeah I plan on doing the grouping in this bug as well. https://hg.mozilla.org/integration/mozilla-inbound/rev/8bec67212ac4
Comment 4•10 years ago
|
||
Great!
Assignee | ||
Comment 5•10 years ago
|
||
These are most of the messages that we relatively easy to group with a script.
Attachment #8474096 -
Flags: review?(till)
Comment 6•10 years ago
|
||
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•10 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.
Comment 8•10 years ago
|
||
(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•10 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•10 years ago
|
||
Attachment #8474140 -
Flags: review?(till)
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8bec67212ac4 https://hg.mozilla.org/mozilla-central/rev/408f6612a5e9
Comment 12•10 years ago
|
||
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•10 years ago
|
||
Almost done here, only 50 or so left to be sorted through. https://hg.mozilla.org/integration/mozilla-inbound/rev/6f78b69406b6
Assignee | ||
Comment 15•10 years ago
|
||
I guess I won't to the rest here anymore.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Keywords: leave-open
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•