Closed Bug 1450085 Opened 2 years ago Closed 2 years ago

Use default arguments for ReportValueError instead of macros and more cleanup

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: anba, Assigned: anba)

Details

Attachments

(3 files)

The |ReportValueError| macros [1] could be changed to proper C++ functions with defaults arguments. For example like so:

---
extern void
ReportValueError(JSContext* cx, unsigned flags, const unsigned errorNumber,
                 int spindex, HandleValue v, HandleString fallback = nullptr,
                 const char* arg1 = nullptr, const char* arg2 = nullptr)
{
    mozilla::Unused << ReportValueErrorFlags(cx, JSREPORT_ERROR, errorNumber, spindex, v, fallback, arg1, arg2);
}
---

After that
- Callers to ReportValueErrorFlags which use |flags = JSREPORT_ERROR|, like [2], could then be changed to call ReportValueError.
- An explicit |nullptr| argument for the |fallback| parameter can then be removed, for example in [3].
- And completely unused functions can be deleted, like [4].


[1] https://searchfox.org/mozilla-central/rev/7e663b9fa578d425684ce2560e5fa2464f504b34/js/src/vm/JSContext.h#1091-1101
[2] https://searchfox.org/mozilla-central/rev/7e663b9fa578d425684ce2560e5fa2464f504b34/js/src/builtin/ReflectParse.cpp#291-292
[3] https://searchfox.org/mozilla-central/rev/7e663b9fa578d425684ce2560e5fa2464f504b34/js/src/builtin/Promise.cpp#900
[4] https://searchfox.org/mozilla-central/rev/7e663b9fa578d425684ce2560e5fa2464f504b34/js/src/vm/JSObject.cpp#2584-2609
Priority: -- → P3
Replaces the |ReportValueError| macros with a proper function and removes the remaining C89-bits from js::ReportValueErrorFlags(...).
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Attachment #8981400 - Flags: review?(tcampbell)
Replaces calls to ReportValueErrorFlags(...) with ReportValueError(...) when the error-flags arguments is JSREPORT_ERROR. This matches how the JSAPI error API "normally" works. (Except when it doesn't -> Part 3!)


JSObject::report{ReadOnly,NotConfigurable,NotExtensible} are no longer used, so instead of updating them to use ReportValueError(...), we can instead remove them completely.
Attachment #8981401 - Flags: review?(tcampbell)
Replaces call to JS_ReportErrorFlagsAndNumber{ASCII,Latin1,UTF8,UC}(...) with JS_ReportErrorNumber{ASCII,Latin1,UTF8,UC}(...) when the error-flags argument is JSREPORT_ERROR.

I've also removed the unused |fallback| argument from js::ReportIsNullOrUndefined(...) and changed the function to use a void-return to match the standard error API pattern.
Attachment #8981402 - Flags: review?(tcampbell)
Attachment #8981400 - Flags: review?(tcampbell) → review+
Attachment #8981401 - Flags: review?(tcampbell) → review+
Attachment #8981402 - Flags: review?(tcampbell) → review+
Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e478d973ae2
Part 1: Change ReportValueError to a proper function instead of a macro. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/9529d02de634
Part 2: Replace ReportValueErrorFlags with ReportValueError where applicable. r=tcampbell
https://hg.mozilla.org/integration/mozilla-inbound/rev/a23ae192f340
Part 3: Call error-functions instead of passing JSREPORT_ERROR flag. r=tcampbell
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3e478d973ae2
https://hg.mozilla.org/mozilla-central/rev/9529d02de634
https://hg.mozilla.org/mozilla-central/rev/a23ae192f340
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.