Last Comment Bug 766347 - It's not possible to pass Rooted<S> to a Foo method if it has Foo(Handle<S>) and Foo(Handle<T>) overloads
: It's not possible to pass Rooted<S> to a Foo method if it has Foo(Handle<S>) ...
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-19 15:14 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-06-23 19:56 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
mozilla::EnableIf<bool, typename> (1.23 KB, patch)
2012-06-19 15:15 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
mozilla::IsConvertible<typename From, typename To> (3.92 KB, patch)
2012-06-19 15:18 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review
Fix the actual bug now (2.39 KB, patch)
2012-06-19 15:20 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-19 15:14:25 PDT
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?
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-19 15:15:19 PDT
Created attachment 634615 [details] [diff] [review]
mozilla::EnableIf<bool, typename>

Apparently the foundational element of SFINAE.  Fun fun fun!
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-19 15:18:40 PDT
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.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-19 15:20:14 PDT
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.  :-)
Comment 4 Luke Wagner [:luke] 2012-06-22 14:10:06 PDT
Comment on attachment 634615 [details] [diff] [review]
mozilla::EnableIf<bool, typename>

u r become death, the destroyer of worlds
Comment 5 Luke Wagner [:luke] 2012-06-22 14:12:36 PDT
Comment on attachment 634617 [details] [diff] [review]
mozilla::IsConvertible<typename From, typename To>

s/D/C/
Comment 6 Luke Wagner [:luke] 2012-06-22 14:17:13 PDT
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'?
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-23 11:10:41 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.