Closed
Bug 1155081
Opened 9 years ago
Closed 9 years ago
Remove ThrowError and replace all its uses with ThrowTypeError/ThrowRangeError/etc.
Categories
(Core :: JavaScript: Standard Library, defect)
Core
JavaScript: Standard Library
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: Waldo, Assigned: arai)
References
Details
Attachments
(13 files)
13.42 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
988 bytes,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
4.54 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
6.29 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
5.93 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
1.50 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
1.43 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
5.08 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
7.70 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
19.37 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
3.81 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
5.04 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
4.69 KB,
patch
|
till
:
review+
|
Details | Diff | Splinter Review |
ThrowError is infuriatingly uninformative about what it's actually throwing. Audit every use of it to determine whether each type thrown is correct, then convert each of them to the right more-specific intrinsic. This work can and should be split up into multiple patches, for reviewer sanity and attentiveness.
Assignee | ||
Comment 1•9 years ago
|
||
Prepared 11 patches, for each js file (2 patches for TypedObject.js), and removing intrinsic. Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=95fe58d349ba
Assignee: nobody → arai.unmht
Attachment #8594465 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8594466 -
Flags: review?(till)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8594467 -
Flags: review?(wingo)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8594468 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 5•9 years ago
|
||
forgot to note that merged Map/Set/WeakMap in one patch.
Attachment #8594469 -
Flags: review?(till)
Assignee | ||
Comment 6•9 years ago
|
||
Attachment #8594470 -
Flags: review?(till)
Assignee | ||
Comment 7•9 years ago
|
||
Attachment #8594471 -
Flags: review?(till)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8594472 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 9•9 years ago
|
||
Attachment #8594473 -
Flags: review?(evilpies)
Assignee | ||
Comment 10•9 years ago
|
||
JSMSG_TYPEDOBJECT_ARRAYTYPE_BAD_ARGS is JSEXN_ERR, and it seems to be a duplicate of JSMSG_TYPEDOBJECT_BAD_ARGS (sorry if not), so I merged it in other patch.
Attachment #8594474 -
Flags: review?(sphink)
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8594475 -
Flags: review?(evilpies)
Assignee | ||
Comment 12•9 years ago
|
||
replaced JSMSG_TYPEDOBJECT_ARRAYTYPE_BAD_ARGS with JSMSG_TYPEDOBJECT_BAD_ARGS.
Attachment #8594476 -
Flags: review?(sphink)
Assignee | ||
Comment 13•9 years ago
|
||
uh oh, there are 13 patches :O
Attachment #8594477 -
Flags: review?(jwalden+bmo)
Comment 14•9 years ago
|
||
Comment on attachment 8594466 [details] [diff] [review] Part 2: Replace ThrowError with ThrowTypeError in Error.js. Review of attachment 8594466 [details] [diff] [review]: ----------------------------------------------------------------- nice
Attachment #8594466 -
Flags: review?(till) → review+
Comment 15•9 years ago
|
||
Comment on attachment 8594469 [details] [diff] [review] Part 5: Replace ThrowError with ThrowTypeError in Map.js/Set.js/WeakSet.js. Review of attachment 8594469 [details] [diff] [review]: ----------------------------------------------------------------- Yes.
Attachment #8594469 -
Flags: review?(till) → review+
Comment 16•9 years ago
|
||
Comment on attachment 8594470 [details] [diff] [review] Part 6: Replace ThrowError with ThrowTypeError in Object.js. Review of attachment 8594470 [details] [diff] [review]: ----------------------------------------------------------------- Certainly.
Attachment #8594470 -
Flags: review?(till) → review+
Comment 17•9 years ago
|
||
Comment on attachment 8594471 [details] [diff] [review] Part 7: Replace ThrowError with ThrowTypeError in RegExp.js. Review of attachment 8594471 [details] [diff] [review]: ----------------------------------------------------------------- This, too.
Attachment #8594471 -
Flags: review?(till) → review+
Comment 18•9 years ago
|
||
Comment on attachment 8594465 [details] [diff] [review] Part 1: Replace ThrowError with ThrowTypeError in Array.js. Review of attachment 8594465 [details] [diff] [review]: ----------------------------------------------------------------- Stealing. Somewhat surprising that these are all TypeErrors, but since they are, sure.
Attachment #8594465 -
Flags: review?(jwalden+bmo) → review+
Comment 19•9 years ago
|
||
Comment on attachment 8594467 [details] [diff] [review] Part 3: Replace ThrowError with ThrowTypeError in Generator.js. Review of attachment 8594467 [details] [diff] [review]: ----------------------------------------------------------------- Stealing this, too. Andy, I know you're itching to get back to SpiderMonkey stuff, but this doesn't seem like it's the right entry point.
Attachment #8594467 -
Flags: review?(wingo) → review+
Comment 20•9 years ago
|
||
Comment on attachment 8594468 [details] [diff] [review] Part 4: Replace ThrowError with ThrowTypeError/ThrowRangeError in Intl.js. Review of attachment 8594468 [details] [diff] [review]: ----------------------------------------------------------------- Finally some non-TypeErrors! Also, stealing.
Attachment #8594468 -
Flags: review?(jwalden+bmo) → review+
Comment 21•9 years ago
|
||
Comment on attachment 8594472 [details] [diff] [review] Part 8: Replace ThrowError with ThrowTypeError/ThrowRangeError in String.js. Review of attachment 8594472 [details] [diff] [review]: ----------------------------------------------------------------- Again, stealing.
Attachment #8594472 -
Flags: review?(jwalden+bmo) → review+
Comment 22•9 years ago
|
||
Comment on attachment 8594473 [details] [diff] [review] Part 9: Replace ThrowError with ThrowTypeError in TypedArray.js. Review of attachment 8594473 [details] [diff] [review]: ----------------------------------------------------------------- Stealing.
Attachment #8594473 -
Flags: review?(evilpies) → review+
Comment 23•9 years ago
|
||
Comment on attachment 8594474 [details] [diff] [review] Part 10: Replace ThrowError for TypeError with ThrowTypeError in TypedObject.js. Review of attachment 8594474 [details] [diff] [review]: ----------------------------------------------------------------- Stealing. r=me with or without nit addressed. ::: js/src/builtin/TypedObject.js @@ +726,5 @@ > assert(IsObject(arrayType) && ObjectIsTypeDescr(arrayType), "Build called on non-type-object"); > > if (depth <= 0 || TO_INT32(depth) !== depth) > // RangeError("bad depth") > + ThrowTypeError(JSMSG_TYPEDOBJECT_BAD_ARGS); Pre-existing nit: this if-statement should have curly braces, because the body is two lines. (Yes, comments count.)
Attachment #8594474 -
Flags: review?(sphink) → review+
Comment 24•9 years ago
|
||
Comment on attachment 8594475 [details] [diff] [review] Part 11: Replace ThrowError with ThrowTypeError in Utilities.js. Review of attachment 8594475 [details] [diff] [review]: ----------------------------------------------------------------- Yes, stealing. ::: js/src/builtin/Utilities.js @@ +10,5 @@ > */ > > /*global ToObject: false, ToInteger: false, IsCallable: false, > + ThrowError: false, ThrowRangeError: false, ThrowTypeError: false, > + AssertionFailed: false, I don't think anybody's actually using a linter that uses these annotations on self-hosted code, but since we have them, we should keep them up-to-date as much as possible, yes. @@ +184,5 @@ > return s; > > // Step 10. > + ThrowTypeError(JSMSG_NOT_CONSTRUCTOR, > + "@@species property of object's constructor"); Pre-existing nit: this fits into one line. (It might well be on two lines on purpose, but I think being consistent with the prevailing style in surrounding code is more important.)
Attachment #8594475 -
Flags: review?(evilpies) → review+
Comment 25•9 years ago
|
||
Comment on attachment 8594476 [details] [diff] [review] Part 12: Replace JSMSG_TYPEDOBJECT_ARRAYTYPE_BAD_ARGS with JSMSG_TYPEDOBJECT_BAD_ARGS in TypedObject.js. Review of attachment 8594476 [details] [diff] [review]: ----------------------------------------------------------------- Good call. You should perhaps include something about the s/ThrowError/ThrowTypeError/ thing in the commit message. Perhaps on a second line. (Yes, these strange beasts are rare, but they do sometimes sneak in.)
Attachment #8594476 -
Flags: review?(sphink) → review+
Comment 26•9 years ago
|
||
Comment on attachment 8594477 [details] [diff] [review] Part 13: Remove ThrowError intrinsic. Review of attachment 8594477 [details] [diff] [review]: ----------------------------------------------------------------- So nice, thank you!
Attachment #8594477 -
Flags: review?(jwalden+bmo) → review+
Comment 27•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb3e1a036aeb https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0d8d5ad861 https://hg.mozilla.org/integration/mozilla-inbound/rev/273ba0af084e https://hg.mozilla.org/integration/mozilla-inbound/rev/efa863af21b3 https://hg.mozilla.org/integration/mozilla-inbound/rev/cb6a170682c3 https://hg.mozilla.org/integration/mozilla-inbound/rev/556318c01358 https://hg.mozilla.org/integration/mozilla-inbound/rev/9253a9d5cb1d https://hg.mozilla.org/integration/mozilla-inbound/rev/156c934d3a66 https://hg.mozilla.org/integration/mozilla-inbound/rev/74ee49392909 https://hg.mozilla.org/integration/mozilla-inbound/rev/757bedb29672 https://hg.mozilla.org/integration/mozilla-inbound/rev/cd3848e68b08 https://hg.mozilla.org/integration/mozilla-inbound/rev/72784246a79d https://hg.mozilla.org/integration/mozilla-inbound/rev/1b636807324d
https://hg.mozilla.org/mozilla-central/rev/eb3e1a036aeb https://hg.mozilla.org/mozilla-central/rev/2a0d8d5ad861 https://hg.mozilla.org/mozilla-central/rev/273ba0af084e https://hg.mozilla.org/mozilla-central/rev/efa863af21b3 https://hg.mozilla.org/mozilla-central/rev/cb6a170682c3 https://hg.mozilla.org/mozilla-central/rev/556318c01358 https://hg.mozilla.org/mozilla-central/rev/9253a9d5cb1d https://hg.mozilla.org/mozilla-central/rev/156c934d3a66 https://hg.mozilla.org/mozilla-central/rev/74ee49392909 https://hg.mozilla.org/mozilla-central/rev/757bedb29672 https://hg.mozilla.org/mozilla-central/rev/cd3848e68b08 https://hg.mozilla.org/mozilla-central/rev/72784246a79d https://hg.mozilla.org/mozilla-central/rev/1b636807324d
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•