Closed Bug 1075564 Opened 5 years ago Closed 5 years ago

I'm sorry I didn't mark my implicit constructor; may I please have it back?

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: jimb, Assigned: jimb)

Details

Attachments

(1 file)

JS::ubi::Node should have an implicit constructor from JS::Value.

In bug 960786, I landed JS::ubi::Node, with an implicit Node(JS::Value) constructor.

In bug 1044596, Ehsan marked JS::ubi::Node::Node(JS::Value) explicit.

In bug 1063247, I made it implicit again.

In bug 1068024, Ehsan marked it as explicit again.

I now understand that implicit constructors should be marked MOZ_IMPLICIT. I approve of this policy; regret having not noticed my transgression earlier; and have rectified my code accordingly.
Attachment #8498187 - Flags: review?(luke)
Comment on attachment 8498187 [details] [diff] [review]
Give JS::ubi::Node its implicit constructor back, with the right annotation this time.

lol
Attachment #8498187 - Flags: review?(luke) → review+
Comment on attachment 8498187 [details] [diff] [review]
Give JS::ubi::Node its implicit constructor back, with the right annotation this time.

Review of attachment 8498187 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/UbiNode.h
@@ +276,5 @@
>      // Constructors accepting SpiderMonkey's other generic-pointer-ish types.
> +    // Note that we *do* want an implicit constructor here: JS::Value and
> +    // JS::ubi::Node are both essentially tagged references to other sorts of
> +    // objects, so letting conversions happen automatically is appropriate.
> +    Node(JS::HandleValue value) MOZ_IMPLICIT;

Nit: please move MOZ_IMPLICIT to the beginning of the declaration.
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #2)
> Nit: please move MOZ_IMPLICIT to the beginning of the declaration.

Done, thanks.
Assignee: nobody → jimb
Flags: in-testsuite-
Target Milestone: --- → mozilla35
https://hg.mozilla.org/mozilla-central/rev/c70ca7e54741
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.