Closed Bug 1013662 Opened 6 years ago Closed 6 years ago

Fix bad implicit conversion constructors in MFBT

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: ehsan, Assigned: ehsan)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
Blocks: explicit
Assignee: nobody → ehsan
Comment on attachment 8425903 [details] [diff] [review]
Fix bad implicit conversion constructors in MFBT

This is bit-rot-stastic!  Fast reviews appreciated.
Attachment #8425903 - Flags: review?(nfroyd)
Comment on attachment 8425903 [details] [diff] [review]
Fix bad implicit conversion constructors in MFBT

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

::: mfbt/Vector.h
@@ +1190,5 @@
>  {
>      typedef VectorBase<T, MinInlineCapacity, AllocPolicy, Vector> Base;
>  
>    public:
> +    explicit Vector(AllocPolicy alloc = AllocPolicy()) : Base(alloc) {}

As I said in the JS engine bug, I am not a fan of forcing explicit naming of the relevant AllocPolicy here.  I would much rather see

    template<typename T>
    explicit VectorBase(T&& t) : Base(AllocPolicy(mozilla::Forward<T>(t))) {}

or so, which I "think" might be adequate here for your purposes.

I should say that while non-explicit constructors don't seem a good thing fairly often, I am not convinced that they are always and everywhere bad and worthy of total prohibition.  But I don't know that there's some particular use that is super-motivated and can't be performed some other way with little to no skin off anyone's nose.  If this AllocPolicy one can't be solved with perfect forwarding as here, possibly it may qualify as an exceptional case.
Attachment #8425903 - Flags: review?(nfroyd)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> Comment on attachment 8425903 [details] [diff] [review]
> Fix bad implicit conversion constructors in MFBT
> 
> Review of attachment 8425903 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mfbt/Vector.h
> @@ +1190,5 @@
> >  {
> >      typedef VectorBase<T, MinInlineCapacity, AllocPolicy, Vector> Base;
> >  
> >    public:
> > +    explicit Vector(AllocPolicy alloc = AllocPolicy()) : Base(alloc) {}
> 
> As I said in the JS engine bug, I am not a fan of forcing explicit naming of
> the relevant AllocPolicy here.  I would much rather see
> 
>     template<typename T>
>     explicit VectorBase(T&& t) : Base(AllocPolicy(mozilla::Forward<T>(t))) {}
> 
> or so, which I "think" might be adequate here for your purposes.

Hmm, by making the ctor for all AllocPolicy classes MOZ_IMPLICIT (which I have already done in bug 1013663) that annoying pattern of typing in the name of the alloc policy class is no longer needed.  Does that make this issue moot?

> I should say that while non-explicit constructors don't seem a good thing
> fairly often, I am not convinced that they are always and everywhere bad and
> worthy of total prohibition.  But I don't know that there's some particular
> use that is super-motivated and can't be performed some other way with
> little to no skin off anyone's nose.  If this AllocPolicy one can't be
> solved with perfect forwarding as here, possibly it may qualify as an
> exceptional case.

That's OK, that's why we have MOZ_IMPLICIT.  It hints to our analysis to make an exception for the given class and is intended to be used for the cases where you really want an implicit conversion here.

FWIW rewriting a bunch of stuff to use perfect forwarding is an interesting and different project.  Given that bug 1009631 is already going to be a good amount of work, I don't really plan to take on this other project as part of that.
Flags: needinfo?(jwalden+bmo)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #4)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> > Comment on attachment 8425903 [details] [diff] [review]
> > Fix bad implicit conversion constructors in MFBT
> > 
> > Review of attachment 8425903 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mfbt/Vector.h
> > @@ +1190,5 @@
> > >  {
> > >      typedef VectorBase<T, MinInlineCapacity, AllocPolicy, Vector> Base;
> > >  
> > >    public:
> > > +    explicit Vector(AllocPolicy alloc = AllocPolicy()) : Base(alloc) {}
> > 
> > As I said in the JS engine bug, I am not a fan of forcing explicit naming of
> > the relevant AllocPolicy here.  I would much rather see
> > 
> >     template<typename T>
> >     explicit VectorBase(T&& t) : Base(AllocPolicy(mozilla::Forward<T>(t))) {}
> > 
> > or so, which I "think" might be adequate here for your purposes.
> 
> Hmm, by making the ctor for all AllocPolicy classes MOZ_IMPLICIT (which I
> have already done in bug 1013663) that annoying pattern of typing in the
> name of the alloc policy class is no longer needed.  Does that make this
> issue moot?

(To make my point clearer, we still don't want a vector to be implicitly constructible from an alloc policy object where the author doesn't intend to do so, right?)
Comment on attachment 8425903 [details] [diff] [review]
Fix bad implicit conversion constructors in MFBT

Can you please at least review the Attributes.h part of this patch while we figure out the rest of the discussion?  I need that to be able to land the rest of my work in other bugs.  Thanks!
Attachment #8425903 - Flags: review?(nfroyd)
Attachment #8425903 - Flags: review?(jwalden+bmo)
Comment on attachment 8425903 [details] [diff] [review]
Fix bad implicit conversion constructors in MFBT

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

r=me on Attributes.h with the spelling fix below.

::: mfbt/Attributes.h
@@ +483,5 @@
>   *   value is allocated on the heap, and will as a result check such allocations
>   *   during MOZ_STACK_CLASS and MOZ_NONHEAP_CLASS annotation checking.
> + * MOZ_IMPLICIT: Applies to constructors. Implicit conversion constructors
> + *   are disallowed by default unless they are marked as MOZ_IMPLICIT. This
> + *   attribute must be used for constructors which intent to provide implicit

"which intend"

::: mfbt/MathAlgorithms.h
@@ +25,5 @@
>  {
>    // Euclid's algorithm; O(N) in the worst case.  (There are better
>    // ways, but we don't need them for the current use of this algo.)
> +  MOZ_ASSERT(a > IntegerType(0));
> +  MOZ_ASSERT(b > IntegerType(0));

Is this just being explicit for the sake of being explicit, or is there something else going on here?
Attachment #8425903 - Flags: review?(nfroyd) → review+
Comment on attachment 8425903 [details] [diff] [review]
Fix bad implicit conversion constructors in MFBT

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

Attributes.h bits are fine with me.  Double-r+ on them.

I am incredibly sandbagged with another bug right now, as you're aware, so the other needinfo/request thingy, and the rest of this one, is in line behind that.
(In reply to Nathan Froyd (:froydnj) from comment #7)
> Comment on attachment 8425903 [details] [diff] [review]
> Fix bad implicit conversion constructors in MFBT
> 
> Review of attachment 8425903 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me on Attributes.h with the spelling fix below.
> 
> ::: mfbt/Attributes.h
> @@ +483,5 @@
> >   *   value is allocated on the heap, and will as a result check such allocations
> >   *   during MOZ_STACK_CLASS and MOZ_NONHEAP_CLASS annotation checking.
> > + * MOZ_IMPLICIT: Applies to constructors. Implicit conversion constructors
> > + *   are disallowed by default unless they are marked as MOZ_IMPLICIT. This
> > + *   attribute must be used for constructors which intent to provide implicit
> 
> "which intend"

English is stupid!  ;-)

> ::: mfbt/MathAlgorithms.h
> @@ +25,5 @@
> >  {
> >    // Euclid's algorithm; O(N) in the worst case.  (There are better
> >    // ways, but we don't need them for the current use of this algo.)
> > +  MOZ_ASSERT(a > IntegerType(0));
> > +  MOZ_ASSERT(b > IntegerType(0));
> 
> Is this just being explicit for the sake of being explicit, or is there
> something else going on here?

I forget how but we somehow specialize this for Decimal, so this is actually required to make the code compile with that change.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> Comment on attachment 8425903 [details] [diff] [review]
> Fix bad implicit conversion constructors in MFBT
> 
> Review of attachment 8425903 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Attributes.h bits are fine with me.  Double-r+ on them.
> 
> I am incredibly sandbagged with another bug right now, as you're aware, so
> the other needinfo/request thingy, and the rest of this one, is in line
> behind that.

Yep, I'll land the Attributes.h part, the rest can wait, although I think I'd like to land this all before I land bug 1013663, and that patch is pretty bitrot prone, so I may poke you on IRC soon.  :-)
Keywords: leave-open
So based on the IRC conversation today, is this patch good to go?
Ping?  Should I just land this as is?
Ping?
Flags: needinfo?(nfroyd)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #14)
> Ping?

I assumed that comment 12 and comment 13 were directed at Waldo, because I don't remember having an IRC conversation with you and there was no more r? for me on the patch.  Is that not the case?
Flags: needinfo?(nfroyd)
(In reply to comment #15)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #14)
> > Ping?
> 
> I assumed that comment 12 and comment 13 were directed at Waldo, because I
> don't remember having an IRC conversation with you and there was no more r? for
> me on the patch.  Is that not the case?

It is.  Just trying to get some attention here, I've already ni?ed him.  :-)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #16)
> It is.  Just trying to get some attention here, I've already ni?ed him.  :-)

Is this like good cop, bad cop?  Which one do I get to be? :)
(In reply to Nathan Froyd (:froydnj) from comment #17)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #16)
> > It is.  Just trying to get some attention here, I've already ni?ed him.  :-)
> 
> Is this like good cop, bad cop?  Which one do I get to be? :)

Neither, I just want to stop rebasing this patch every day. :(  Will try to catch Jeff on IRC, he's not on right now...
Comment on attachment 8425903 [details] [diff] [review]
Fix bad implicit conversion constructors in MFBT

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

Yes, go ahead with this.  But file the followup to make the Vector bit explicit again, please -- it really shouldn't be that hard.
Attachment #8425903 - Flags: review?(jwalden+bmo) → review+
Flags: needinfo?(jwalden+bmo)
Blocks: 1022087
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #19)
> Comment on attachment 8425903 [details] [diff] [review]
> Fix bad implicit conversion constructors in MFBT
> 
> Review of attachment 8425903 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes, go ahead with this.  But file the followup to make the Vector bit
> explicit again, please -- it really shouldn't be that hard.

Done: bug 1022087.  Thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7dc39d16234
https://hg.mozilla.org/mozilla-central/rev/d7dc39d16234
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1245379
You need to log in before you can comment on or make changes to this bug.