The default bug view has changed. See this FAQ.

It's not possible to pass Rooted<S> to a Foo method if it has Foo(Handle<S>) and Foo(Handle<T>) overloads

RESOLVED FIXED in mozilla16

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(3 attachments)

The problem is that Handle<T> can be constructed from Rooted<S> for any S.  The testAssign method is supposed to enforce convertibility, but it doesn't affect type signatures in a way that triggers correct argument conversion.  You know what that means: SFINAE time!  Excited yet?
Created attachment 634615 [details] [diff] [review]
mozilla::EnableIf<bool, typename>

Apparently the foundational element of SFINAE.  Fun fun fun!
Attachment #634615 - Flags: review?(luke)
Created attachment 634617 [details] [diff] [review]
mozilla::IsConvertible<typename From, typename To>

Also includes tests.  It doesn't work quite right in one far edge case, commented out in the test, but if someone were trying that, I'm pretty sure they'd be trying to be dumb, more or less.
Attachment #634617 - Flags: review?(luke)
Created attachment 634618 [details] [diff] [review]
Fix the actual bug now

I plan to add some users soon, but I haven't added any here.  I did test that a use did work with this patch.

I haven't tryservered any of these patches, but I'll be doing so now.  :-)
Attachment #634618 - Flags: review?(luke)
Whiteboard: [js:t]

Comment 4

5 years ago
Comment on attachment 634615 [details] [diff] [review]
mozilla::EnableIf<bool, typename>

u r become death, the destroyer of worlds
Attachment #634615 - Flags: review?(luke) → review+

Comment 5

5 years ago
Comment on attachment 634617 [details] [diff] [review]
mozilla::IsConvertible<typename From, typename To>

s/D/C/
Attachment #634617 - Flags: review?(luke) → review+

Comment 6

5 years ago
Comment on attachment 634618 [details] [diff] [review]
Fix the actual bug now

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

1337 C++ h4x0r

::: js/src/gc/Root.h
@@ +78,5 @@
> +    /* Creates a handle from a handle of a type convertible to T. */
> +    template <typename S>
> +    Handle(Handle<S> handle,
> +           typename mozilla::EnableIf<mozilla::IsConvertible<S, T>::value,
> +                                      int>::Type dummy = 0)

This line-break is awkward.  Can you hoist the IsConvertible expression up to a static const bool and reuse in the other two constructor decls.

@@ +104,5 @@
> +    template <typename S>
> +    inline
> +    Handle(const Rooted<S> &root,
> +           typename mozilla::EnableIf<mozilla::IsConvertible<S, T>::value,
> +                                      int>::Type dummy = 0);

I think you already did this in a prev patch, but could you remove the 'const'?
Attachment #634618 - Flags: review?(luke) → review+
The IsConvertible isn't hoistable because it depends on typename S, but those lines were short enough to fit in 99ch anyway, so I removed the linebreaks.

I had these patches in my queue before the const removal one, but I ended up reordering my queue to land that one first, so no const to remove any more.

https://hg.mozilla.org/integration/mozilla-inbound/rev/54a17b76f488
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe817bf85f36
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0fd1627db78

And yes, death.
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/54a17b76f488
https://hg.mozilla.org/mozilla-central/rev/fe817bf85f36
https://hg.mozilla.org/mozilla-central/rev/b0fd1627db78
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.