1116905 - Remove the explicit conversion from T* to TemporaryRef<T>

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[(no longer active)]

This is in preparation of removing TemporaryRef.  It should help make
already_AddRefed a drop-in replacement for it.

Comment 1

[(no longer active)]

Attachment: Remove the explicit conversion from T* to TemporaryRef<T>

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8543119 [details] [diff] [review]
Remove the explicit conversion from T* to TemporaryRef<T>

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

I think this approach is not really an improvement in terms of code readability. Can we come up with something that's more explicit, concise and idiomatic? Maybe something like MakeRefPtr<T>(constructor arguments) to parallel MakeUnique<T>.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> I think this approach is not really an improvement in terms of code
> readability. Can we come up with something that's more explicit, concise and
> idiomatic? Maybe something like MakeRefPtr<T>(constructor arguments) to
> parallel MakeUnique<T>.

So you'd rather see all these cases written as:

  RefPtr(aVarName).forget()

?

I guess the problem here is that there's a mismatch between TemporaryRef's constructor and already_AddRefed's constructor for raw pointer values: the former implicitly takes ownership, and the latter does not.  Maybe we could call this |static already_AddRefed already_AddRefed::own|?

Comment 4

[(no longer active)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> I think this approach is not really an improvement in terms of code
> readability. Can we come up with something that's more explicit, concise and
> idiomatic? Maybe something like MakeRefPtr<T>(constructor arguments) to
> parallel MakeUnique<T>.

That seems to be outside of the scope of bug 1115033.  Also, this is not about RefPtr at all, it is about TemporaryRef.

(FWIW I agree that what you're suggesting is better, just pointing out that it's irrelevant to the patch at hand.)

Comment 5

[(no longer active)]

ping?

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #5)
> ping?

I was awaiting Jeff's reply, since gfx/ code is ~half the patch.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #3)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> > I think this approach is not really an improvement in terms of code
> > readability. Can we come up with something that's more explicit, concise and
> > idiomatic? Maybe something like MakeRefPtr<T>(constructor arguments) to
> > parallel MakeUnique<T>.
> 
> So you'd rather see all these cases written as:
> 
>   RefPtr(aVarName).forget()
> 
> ?

No.

I'd rather something like:

static already_Addrefed<T> MakeRefPtr<T>(...)

or 

static RefPtr<T> MakeRefPtr<T>(...)

so that these cases become:

return MakeRefPtr<PathD2D>(constructor arguments);

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> I'd rather something like:
> 
> static already_Addrefed<T> MakeRefPtr<T>(...)

MakeRefPtr doesn't seem like a good name for something that returns already_AddRefed.</bikeshed>

> so that these cases become:
> 
> return MakeRefPtr<PathD2D>(constructor arguments);

I could get behind that.  (Sorry, I think I was being dense when I read your first comment.)

Comment 9

[Nathan Froyd [:froydnj]]

Comment on attachment 8543119 [details] [diff] [review]
Remove the explicit conversion from T* to TemporaryRef<T>

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

This is fine as far as it goes, but it needs something for the common |return new T(...)| case.

I propose (variadic-ness left as an exercise for the reader):

template<typename T>
struct ForceNamedType
{
  typedef T type;
};

template<typename T, typename Arg>
already_AddRefed<typename ForceNamedType<T>::type>
MakeAndAddRef(Arg&& a)
{
   RefPtr<T> p(new T(Forward<Arg>(a)));
   return p.forget();
}

which should enable:

  return new T(...);

to be written as:

  return MakeAndAddRef<T>(...);

Bikeshedding on the name welcome.

Comment 10

[Nathan Froyd [:froydnj]]

Waldo, do you have any better suggestions for the name in comment 9?

Ehsan, are you also going to remove RefPtr's implicit conversion to TemporaryRef (as that will also cause problems for switching over to already_AddRefed)?

Comment 11

[(no longer active)]

Can we please not scope creep this bug to cover MakeXXX()?  The list of improvements that we need to make to the status quo is substantially larger than MakeXXX.

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #10)
> Ehsan, are you also going to remove RefPtr's implicit conversion to
> TemporaryRef (as that will also cause problems for switching over to
> already_AddRefed)?

Yes, my plan is to modify TemporaryRef's semantics gradually to make it have the exact same interface and semantics as already_AddRefed and sed it away.  This bug is one step in that direction, and there's more, removing RefPtr's implicit conversion to TemporaryRef is another one.

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #11)
> Can we please not scope creep this bug to cover MakeXXX()?  The list of
> improvements that we need to make to the status quo is substantially larger
> than MakeXXX.

I don't see this as scope creep.  This bug is about removing an implicit conversion, so we need to replace code which uses those implicit conversions with something more explicit.  The proposed patch does it one way, and Jeff has objected to that way as being needlessly verbose.  There are less verbose ways to do it, one of which is proposed in comment 9.  The disagreement here is about what the best notation for the necessary changes is.

Your description of MakeFoo as "scope creep" in this instance comes across to me as "these are changes that I don't want to implement, so I'm going to attempt to dismiss them."  I'm sure that's not what you mean, though!  Can you explain why you see the MakeFoo changes as scope creep?

Comment 13

[(no longer active)]

I thought comment 4 mentions very explicitly that I *agree* with the suggestion.  Again, my goal here is bug 1115033, and the suggestion here does not help with that in any way.  But I don't think debating this back and forth makes much sense.  I'll wait for Waldo's response and just do what you guys are asking.

Comment 14

[(no longer active)]

waldo: ping?

Comment 15

[(no longer active)]

I doubt I'm going to have enough time to finish this before I leave given the lack of response here.

Comment 16

[Nathan Froyd [:froydnj]]

So I talked with Ehsan a bit on IRC, and thought about things over the weekend.  Ehsan's complaint about scope-creep was not so much that he was being asked to rewrite parts of his patch, but that implementing comment 9 as written with already_AddRefed would involve a bunch of extra work verifying the semantics of the callers of the rewritten functions.  TemporaryRef and already_AddRefed have different creation semantics and likely as not different usage patterns that would make this tedious.  (This work, of course, would need to be done at some point, but Ehsan's contention was that it would be better to make that sort of change all at once, rather than doing bits of it piecemeal.)

My concern is that if we left the original patch as-is, then we'd wind up with these undesirable-to-graphics-folks RefPtr/.forget() lines, and it would be tedious to go back and fix them.

So how about a compromise position: we use:

template<typename T, typename Arg>
TemporaryRef<typename ForceNamedType<T>::type>
MakeAndAddRef(Arg&& a)
{
   RefPtr<T> p(new T(Forward<Arg>(a)));
   return p.forget();
}

appropriately-modified for variadic template usage.  That should make the requirement of needing to analyze all the callers for correctness, while still cleaning up things to make graphics folks happy.  Does that sound reasonable and less scope-creepy?

Comment 17

[Jeff Muizelaar [:jrmuizel]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #16)
> template<typename T, typename Arg>
> TemporaryRef<typename ForceNamedType<T>::type>
> MakeAndAddRef(Arg&& a)

I, unsurprisingly, like this idea.

Comment 18

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #16)
> template<typename T, typename Arg>
> TemporaryRef<typename ForceNamedType<T>::type>
> MakeAndAddRef(Arg&& a)
> {
>    RefPtr<T> p(new T(Forward<Arg>(a)));
>    return p.forget();
> }
> 
> appropriately-modified for variadic template usage.  That should make the
> requirement of needing to analyze all the callers for correctness, while
> still cleaning up things to make graphics folks happy.  Does that sound
> reasonable and less scope-creepy?

This was one of the first things I thought about, but I don't think it's reasonable, since you cannot overload functions based on return types, so we can never never have a MakeAndAddRef overload which would return an already_AddRefed side by side with the above, which would mean either of the following scenarios would happen:

1. Either we'd have to invent a MakeAndAddRef2 function which would return an already_AddRefed (which would suck even if we come up with a proper name for it.)
2. Or we would be locked in a corner without the ability to selectively change some TemporaryRef's to already_AddRefed's because some of those conversions will leave us back with the syntax that Jeff dislikes here.

#2 above is especially undesirable to me.  And I have no reason to believe that #1 will fly even if people shrug right now (as it is purely a syntax matter that we are debating right now.)

Comment 19

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail, needinfo? me!) from comment #18)
> This was one of the first things I thought about, but I don't think it's
> reasonable, since you cannot overload functions based on return types, so we
> can never never have a MakeAndAddRef overload which would return an
> already_AddRefed side by side with the above, which would mean either of the
> following scenarios would happen:
> 
> 1. Either we'd have to invent a MakeAndAddRef2 function which would return
> an already_AddRefed (which would suck even if we come up with a proper name
> for it.)

My unstated assumption was that we would not do the same thing for already_AddRefed--at least not until we're ready to remove TemporaryRef usage from the tree.  gfx code seems to be somewhat different than most other Gecko code in this area, and (at least in my limited reading of Gecko code) the patterns in gfx code that require this tend to not come up in other Gecko code.  And if they do, nobody has complained (loudly, publically, AFAICT) about the extra line or two of code that the pattern would require thus far.

Alternatively, we could call one MakeTemporaryRef and the other MakeAlreadyAddRefed, though those names aren't a huge improvement.

> 2. Or we would be locked in a corner without the ability to selectively
> change some TemporaryRef's to already_AddRefed's because some of those
> conversions will leave us back with the syntax that Jeff dislikes here.

Why would this selective conversion be useful?  So we can convert individual directories or other blocks of code away from TemporaryRef without doing tree-wide conversions?

Comment 20

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #19)
> (In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail,
> needinfo? me!) from comment #18)
> > This was one of the first things I thought about, but I don't think it's
> > reasonable, since you cannot overload functions based on return types, so we
> > can never never have a MakeAndAddRef overload which would return an
> > already_AddRefed side by side with the above, which would mean either of the
> > following scenarios would happen:
> > 
> > 1. Either we'd have to invent a MakeAndAddRef2 function which would return
> > an already_AddRefed (which would suck even if we come up with a proper name
> > for it.)
> 
> My unstated assumption was that we would not do the same thing for
> already_AddRefed--at least not until we're ready to remove TemporaryRef
> usage from the tree.

I think your assumption is way too optimistic.  :-)

I think most people think that if something is in MFBT, it is a good idea to use it.  Which makes it really unappealing to add RefPtr specific niceties like this.

But more importantly, I was talking about the case where in the future I end up being proven wrong and we have to do the final conversion more piecemeal.  Then we would end up with the converted parts of the code using the same coding pattern that is being objected to here, unless if we add an already_AddRefed-returning counterpart.

> gfx code seems to be somewhat different than most
> other Gecko code in this area, and (at least in my limited reading of Gecko
> code) the patterns in gfx code that require this tend to not come up in
> other Gecko code.

Quite to the contrary.  This patten comes up all the time elsewhere in Gecko, and every body seems to be fine with it.  I didn't really invent this, I just typed in what happens in dom/, accessible/, and elsewhere.

> And if they do, nobody has complained (loudly,
> publically, AFAICT) about the extra line or two of code that the pattern
> would require thus far.

Indeed.

> Alternatively, we could call one MakeTemporaryRef and the other
> MakeAlreadyAddRefed, though those names aren't a huge improvement.
> 
> > 2. Or we would be locked in a corner without the ability to selectively
> > change some TemporaryRef's to already_AddRefed's because some of those
> > conversions will leave us back with the syntax that Jeff dislikes here.
> 
> Why would this selective conversion be useful?  So we can convert individual
> directories or other blocks of code away from TemporaryRef without doing
> tree-wide conversions?

That, or maybe per file, or per class, or some such.  I really don't know, it's extremely hard to predict the problems that will occur in practice before we get to that stage.

FWIW I am way more interested in the long term gains that we would get from bug 1115033 if someone finishes the work.  The trade-offs just don't align as long as we're prioritizing short term style nits against long term goals of that bug.  I still can't think of a way to balance the two.

Comment 21

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail, needinfo? me!) from comment #20)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #19)
> > (In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail,
> > needinfo? me!) from comment #18)
> > > 1. Either we'd have to invent a MakeAndAddRef2 function which would return
> > > an already_AddRefed (which would suck even if we come up with a proper name
> > > for it.)
> > 
> > My unstated assumption was that we would not do the same thing for
> > already_AddRefed--at least not until we're ready to remove TemporaryRef
> > usage from the tree.
> 
> I think your assumption is way too optimistic.  :-)

I am full of sunshine and optimism when it comes to this bug. :)

> I think most people think that if something is in MFBT, it is a good idea to use
> it.  Which makes it really unappealing to add RefPtr specific niceties like this.

I disagree that it would be a RefPtr-specific nicety.  It certainly would be in the short term, because that's all TemporaryRef cooperates with.  But in the longer term, it would use already_AddRefed, TemporaryRef would be hidden in |$VCS log|, and already_AddRefed would cooperate with RefPtr (deprecated though RefPtr might be), and XPCOM smart pointers, which would make it usable for Gecko developers too.  And Gecko developers might even be happy about it.

> But more importantly, I was talking about the case where in the future I end
> up being proven wrong and we have to do the final conversion more piecemeal.
> Then we would end up with the converted parts of the code using the same
> coding pattern that is being objected to here, unless if we add an
> already_AddRefed-returning counterpart.

But if we get to that point and we were just using bare .forget() everwhere, we already would have to do something ugly, because we can't overload RefPtr::forget() on its return type, right?  So we'd have to add RefPtr::forget_aready_AddRefed or something anyway?  It seems slightly better to have:

class RefPtr {
  friend WhateverWeCallTheAlreadyAddRefedTemplateThing;
  ...
private:
  forget_alreadyAddRefed() { /* like forget(), but different */ }
  ...
};

WhateverWeCallTheAlreadyAddRefedTemplateThing(...) {
  RefPtr<T> p = new T(...);
  return p.forget_alreadyAddRefed();
}

than to have bare calls to forget_alreadyAddRefed sprinkled around the code.

It's possible we could be overly clever and slightly ugly:

template<typename T, template<typename U> Ref = TemporaryRef>
Ref<T>
MakeAndAddRef(...)
{
  ...
}

to specify which kind of temporary reference we're using, rather than inventing a whole 'nother function.  (Or specialize on some enum, or whatever.)

> > gfx code seems to be somewhat different than most
> > other Gecko code in this area, and (at least in my limited reading of Gecko
> > code) the patterns in gfx code that require this tend to not come up in
> > other Gecko code.
> 
> Quite to the contrary.  This patten comes up all the time elsewhere in
> Gecko, and every body seems to be fine with it.  I didn't really invent
> this, I just typed in what happens in dom/, accessible/, and elsewhere.

Fair, but Gecko developers are noted for their high incidence of Stockholm syndrome. ;)  It's been hard to contemplate something like MakeAndAddRef, because not having variadic templates and/or not borrowing tools like Chromium's |pump| means that it's difficult and tedious to write.

> > > 2. Or we would be locked in a corner without the ability to selectively
> > > change some TemporaryRef's to already_AddRefed's because some of those
> > > conversions will leave us back with the syntax that Jeff dislikes here.
> > 
> > Why would this selective conversion be useful?  So we can convert individual
> > directories or other blocks of code away from TemporaryRef without doing
> > tree-wide conversions?
> 
> That, or maybe per file, or per class, or some such.  I really don't know,
> it's extremely hard to predict the problems that will occur in practice
> before we get to that stage.

Agreed.

Comment 22

[(no longer active)]

We talked about this on IRC, and we agreed to have two overloads if we ever need to have both overloads:

MakeAndAddRefAsTemporaryRef and MakeAndAddRefAsAlreadyAddRefed

That being said, I won't be around for three weeks, and I doubt to pick this up when I get back...  So resetting the assignee.

Comment 23

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari [Away: 1/29-2/20] (not reading bugmail, needinfo? me!) from comment #22)
> We talked about this on IRC, and we agreed to have two overloads if we ever
> need to have both overloads:
> 
> MakeAndAddRefAsTemporaryRef and MakeAndAddRefAsAlreadyAddRefed

Yup.

> That being said, I won't be around for three weeks, and I doubt to pick this
> up when I get back...  So resetting the assignee.

=/

Comment 24

[Nathan Froyd [:froydnj]]

Jeff's input is not really needed anymore.

Comment 25

[Nathan Froyd [:froydnj]]

Attachment: part 1 - remove dependence on implicit conversion to TemporaryRef, non-gfx changes

I guess it's kind of weird for Ehsan to review changes that Ehsan originally
wrote, but whatever...

Comment 26

[Nathan Froyd [:froydnj]]

Attachment: part 2 - add MakeAndAddRef helper function to facilitate constructing TemporaryRef

With implicit conversion to TemporaryRef going away, one can no longer write:

  return new T(...);

in a function returning TemporaryRef<T>.  Instead, provide MakeAndAddRef
to prevent people from having to construct boilerplate RefPtrs or
similar.  It also makes converting from TemporaryRef to already_AddRefed
somewhat easier.

Comment 27

[Nathan Froyd [:froydnj]]

Attachment: part 3 - remove dependence on implicit conversion to TemporaryRef, gfx changes

Jeff gets to look at these to see if he likes the ergonomics of MakeAndAddRef
any better.

Comment 28

[Nathan Froyd [:froydnj]]

Attachment: part 4 - remove implicit conversion from non-nullptr T* to TemporaryRef<T>

And the piece de resistance.

Comment 29

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8600028 [details] [diff] [review]
part 3 - remove dependence on implicit conversion to TemporaryRef, gfx changes

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

I do.

Comment 30

[(no longer active)]

Comment on attachment 8600025 [details] [diff] [review]
part 1 - remove dependence on implicit conversion to TemporaryRef, non-gfx changes

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

This looks familiar.  ;-)

Comment 31

[Nathan Froyd [:froydnj]]

Chris, are dom/media/ patches still being uplifted with any regularity?  There's a number of media-specific changes here and in dependent bugs; do those need to be split out and committed separately?

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #31)
> Chris, are dom/media/ patches still being uplifted with any regularity? 
> There's a number of media-specific changes here and in dependent bugs; do
> those need to be split out and committed separately?

Please don't hold back landing due to uplifts; we can resolve conflicts pretty easily here I think.

Important MSE and EME fixes are still being uplifted to 39, but I don't think there will be anything made too hard to uplift by landing the media changes here as-is. We can always uplift the subset of the changes here when we determine it makes our lives easier.

Comment 33

[Nathan Froyd [:froydnj]]

Comment on attachment 8600027 [details] [diff] [review]
part 2 - add MakeAndAddRef helper function to facilitate constructing TemporaryRef

Ms2ger, do you feel like reviewing this?  You are still listed as an MFBT peer. :p

Comment 34

[Nathan Froyd [:froydnj]]

Apparently part 3 causes valgrind builds to fail due to an uninitialized variable warning (but not regular Linux builds):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=804aef6601ea (entire patchset)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=556d9fecaf5d (valgrind only)

Valgrind builds on m-c/m-i complain about the same line as the builds above, but for whatever reason, those builds only make it a warning.  (Perhaps we're using a different version of GCC on our Valgrind builds than on our normal builds?)

It's complaining about:

https://dxr.mozilla.org/mozilla-central/source/gfx/2d/Path.cpp#445

and it certainly looks like the call to FindInflectionPoints a few lines earlier can leave t2 uninitialized:

https://dxr.mozilla.org/mozilla-central/source/gfx/2d/Path.cpp#358

I should think the conditional a few lines earlier after the call to FindInflectionPoints:

https://dxr.mozilla.org/mozilla-central/source/gfx/2d/Path.cpp#440

can use t2 uninitialized as well (should be testing |count == 1| before checking the range of t2), but perhaps the compiler can't see through the maze of jumps?

Jeff, is it safe to simply initialize t2 (and maybe t1, for consistency) to 0.0f to work around the warning?

Comment 35

[Jeff Muizelaar [:jrmuizel]]

It would be safer to set t2 to t1 inside FindInflectionPoints.

Comment 36

[Jeff Muizelaar [:jrmuizel]]

I filed bug 1166879 to deal with the use on #440.
The idea behind FindInflectionPoints is that 'count' describes which of t2 and t1 are initialized, are there places where this code actually does wrong or is gcc confused?

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27c4c70a437e
https://hg.mozilla.org/integration/mozilla-inbound/rev/43684b88e52e
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1f569c21102
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1c86100dbce

Comment 38

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/27c4c70a437e
https://hg.mozilla.org/mozilla-central/rev/43684b88e52e
https://hg.mozilla.org/mozilla-central/rev/f1f569c21102
https://hg.mozilla.org/mozilla-central/rev/b1c86100dbce