Closed Bug 1538779 Opened 1 year ago Closed 1 year ago

Make `Rooted<MyContainer> c(cx)` the equivalent of `Rooted<MyContainer> c(cx, MyContainer(cx))` if possible

Categories

(Core :: JavaScript: GC, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: sfink, Assigned: sfink)

References

Details

Attachments

(2 files)

I don't know if this is really desirable, but it was bothering me that we needed a separate type for Rooted&lt;StackGCVector&lt;Value>> v(cx) avoid v(cx, cx) or v(cx, StackedGCVector&lt;Value>(cx). Since i wasted a bunch of time recently on a harder template metaprogramming problem (and eventually gave up), I thought this seemed easier so maybe I should try it out. What do you guys think?

Priority: -- → P5
This is what it ended up as. I tried to use std::is_constructible<T>, but it produced some weird errors that I didn't want to figure out.

Oh, neat! This has also bugged me for ages, but I couldn't see how to do it.

Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d6f935d90da4
Make `Rooted<MyContainer> c(cx)` the equivalent of `Rooted<MyContainer> c(cx, MyContainer(cx))` if possible r=jwalden

Ugh. The shell build succeeds, but the browser build fails. It's the fault of the update for Waldo's review comment: dispatching to Rooted(cx, SafelyInitialized<T>()) rather than repeating the code. And that revealed a rat.

The issue is that GCPolicy<T> now requires an isValid() function, which I just recently discovered and documented in bug 1534440. Except that it turns out that I'm wrong -- few if any of the Gecko specializations of GCPolicy include that member, and everything works just fine.

It turns out that isValid is only required if you use the Rooted(cx, S&& initial) constructor. If you don't, then no body with a call to isValid() is ever generated. And apparently that's the case with all of the Gecko users.

jonco: I can see 3 ways to proceed: (1) add isValid to everything in Gecko; (2) revert to my previous version that conditionally requires isValid and update the docs; or (3) do messy template magic to test for the existence of isValid to essentially default isValid to { return true; }.

#2 would be no better nor worse than the status quo, but it does make for a bit of a surprising footgun when someone attempts to do Rooted r(cx, mytype(...)) and it fails to compile.

#3 is nicer, except that I've already added a bunch of confusing goop to Rooted's code so I'm not sure if it's a good idea to add more. It can at least be done externally though (by defining a GCPolicyWithIsValid struct that does the weird defaulting stuff, and then just s/GCPolicy::isValid/GCPolicyWithIsValid::isValid/ in the actual Rooted body.)

I'll put up a patch for #3 and see what jonco thinks of it.

Flags: needinfo?(sphink)

(In reply to Steve Fink [:sfink] [:s:] from comment #6)
There are only four uses in Gecko. I'd say just define it for them.

Blocks: 1547782
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f2a3fb166dfb
Make `Rooted<MyContainer> c(cx)` the equivalent of `Rooted<MyContainer> c(cx, MyContainer(cx))` if possible r=jonco

There were also assertion failures (detail::GCPolicyWithIsValid<T>::isValid(ptrRootingAPI)) on RootingAPI.h
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=243377112&repo=autoland&lineNumber=893

Yeah, apparently "it compiles" is not a good enough test for this.

The problem stems from

typename = decltype(T(std::declval&lt;RootingContext>()))

This is supposed to be a test for the existence of a constructor T::T(cx). I thought it was equivalent to code like

T dummy(cx);

which correctly fails. But it's actually more like

decltype(T(cx)) dummy;

which somehow succeeds. (And initializes a JSAtom* to the value of a JSContext*, which then fails the isValid check.) I could use

new T(cx);

which would (correctly) fail, but that only makes sense for pointers, and making it work for other things would be reinventing std::is_constructible. So at this point, I'll just use std::is_constructible, which now... just works. I have no idea why that failed for me last time I used this.

Flags: needinfo?(sphink)
Attachment #9057068 - Attachment description: Bug 1538779 - Make `Rooted<MyContainer> c(cx)` the equivalent of `Rooted<MyContainer> c(cx, MyContainer(cx))` if possible r=jwalden → Bug 1538779 - Make `Rooted<MyContainer> c(cx)` the equivalent of `Rooted<MyContainer> c(cx, MyContainer(cx))` if possible r=jonco
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e57ad617753c
Make `Rooted<MyContainer> c(cx)` the equivalent of `Rooted<MyContainer> c(cx, MyContainer(cx))` if possible r=jonco
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.