1013664 - Fix some bad implicit conversion constructors in XPCOM

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Comment 1

[(no longer active)]

Attachment: Fix bad implicit conversion constructors in XPCOM

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8425910 [details] [diff] [review]
Fix bad implicit conversion constructors in XPCOM

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

::: editor/txtsvc/src/nsTextServicesDocument.cpp
@@ -79,5 @@
>  #undef TS_ATOM
>  
>  nsTextServicesDocument::nsTextServicesDocument()
>  {
> -  mRefCnt         = 0;

O.o

Comment 3

[Nathan Froyd [:froydnj]]

It bothers me slightly that private no-op constructors must explicitly be tagged explicit.  Can we relax that rule?  I realize it opens a small window of problems, but at least the problems are confined to single classes...

Comment 4

[Benjamin Smedberg]

Why does that bother you?

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> Why does that bother you?

We're deliberately marking the constructors as private because we never intend for them to be implemented or used.  Sticking |explicit| on them makes it look as though you are going to need them for something.  The logic of having an |explicit| never-used thing is...odd.

Maybe we should have the MFBT equivalent of chromium's DISALLOW_EVIL_CONSTRUCTORS?  Then we can just stick the |explicit| in that macro and be done with it.  And we can throw in things like MOZ_DELETE, too, which none of these cases use (I realize these are older codes...).

Comment 6

[(no longer active)]

(In reply to comment #5)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> > Why does that bother you?
> 
> We're deliberately marking the constructors as private because we never intend
> for them to be implemented or used.

Well, remember that the compiler can't tell that a ctor is not supposed to be used within the context of a translation unit.  So I won't be able to detect these in my analysis.

> Sticking |explicit| on them makes it look
> as though you are going to need them for something.  The logic of having an
> |explicit| never-used thing is...odd.

I don't view it as this way.  I think explicit should be the default for all ctors, and you need to have a good reason to have a ctor be |implicit|.

> Maybe we should have the MFBT equivalent of chromium's
> DISALLOW_EVIL_CONSTRUCTORS?  Then we can just stick the |explicit| in that
> macro and be done with it.  And we can throw in things like MOZ_DELETE, too,
> which none of these cases use (I realize these are older codes...).

Sure, that WFM but I'm not volunteering.  ;-)

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #6)
> (In reply to comment #5)
> > (In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> > > Why does that bother you?
> > 
> > We're deliberately marking the constructors as private because we never intend
> > for them to be implemented or used.
> 
> Well, remember that the compiler can't tell that a ctor is not supposed to
`> be used within the context of a translation unit.  So I won't be able to
> detect these in my analysis.

I'd expect you can detect the ctor is marked as deleted though, and requiring explicitly deleted ctors to be explicit seems kind of silly.

> > Sticking |explicit| on them makes it look
> > as though you are going to need them for something.  The logic of having an
> > |explicit| never-used thing is...odd.
> 
> I don't view it as this way.  I think explicit should be the default for all
> ctors, and you need to have a good reason to have a ctor be |implicit|.

sure, if you change the language to make the default be explicit then an explicit implicit marking is silly, but the language isn't that way so saying this is explicit but actually can't ever be used for anything is just a waste of space and reading time.

> > Maybe we should have the MFBT equivalent of chromium's
> > DISALLOW_EVIL_CONSTRUCTORS?  Then we can just stick the |explicit| in that
> > macro and be done with it.  And we can throw in things like MOZ_DELETE, too,
> > which none of these cases use (I realize these are older codes...).
> 
> Sure, that WFM but I'm not volunteering.  ;-)

How about having the analysis ignore deleted ctors, and then just marking these as deleted? then leave the macro for someone else.

Comment 8

[(no longer active)]

(In reply to comment #7)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #6)
> > (In reply to comment #5)
> > > (In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> > > > Why does that bother you?
> > > 
> > > We're deliberately marking the constructors as private because we never intend
> > > for them to be implemented or used.
> > 
> > Well, remember that the compiler can't tell that a ctor is not supposed to
> `> be used within the context of a translation unit.  So I won't be able to
> > detect these in my analysis.
> 
> I'd expect you can detect the ctor is marked as deleted though, and requiring
> explicitly deleted ctors to be explicit seems kind of silly.

You are right, and that is exactly what my analysis does already.  :-)

> > > Sticking |explicit| on them makes it look
> > > as though you are going to need them for something.  The logic of having an
> > > |explicit| never-used thing is...odd.
> > 
> > I don't view it as this way.  I think explicit should be the default for all
> > ctors, and you need to have a good reason to have a ctor be |implicit|.
> 
> sure, if you change the language to make the default be explicit then an
> explicit implicit marking is silly, but the language isn't that way so saying
> this is explicit but actually can't ever be used for anything is just a waste
> of space and reading time.

I'm not sure what you're suggesting here, Trevor.  Are you saying that you would like the analysis to magically ignore some constructors just because a human reader of the codee understands that the ctor cannot be abused?  I have no idea how to write an analysis that detects this.

> > > Maybe we should have the MFBT equivalent of chromium's
> > > DISALLOW_EVIL_CONSTRUCTORS?  Then we can just stick the |explicit| in that
> > > macro and be done with it.  And we can throw in things like MOZ_DELETE, too,
> > > which none of these cases use (I realize these are older codes...).
> > 
> > Sure, that WFM but I'm not volunteering.  ;-)
> 
> How about having the analysis ignore deleted ctors, and then just marking these
> as deleted? then leave the macro for someone else.

Like I said, the analysis already does ignore deleted ctors.  But please remember that you're asking me to investigate two things per tens of thousands of constructors in our code base, just to make the |explicit| keyword replaced by |MOZ_DELETE|.  That doesn't seem worth the effort to me.

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #8)
> (In reply to comment #7)
> > (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> > #6)
> > > (In reply to comment #5)
> > > > (In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> > > > > Why does that bother you?
> > > > 
> > > > We're deliberately marking the constructors as private because we never intend
> > > > for them to be implemented or used.
> > > 
> > > Well, remember that the compiler can't tell that a ctor is not supposed to
> > `> be used within the context of a translation unit.  So I won't be able to
> > > detect these in my analysis.
> > 
> > I'd expect you can detect the ctor is marked as deleted though, and requiring
> > explicitly deleted ctors to be explicit seems kind of silly.
> 
> You are right, and that is exactly what my analysis does already.  :-)

ok

> > > > Sticking |explicit| on them makes it look
> > > > as though you are going to need them for something.  The logic of having an
> > > > |explicit| never-used thing is...odd.
> > > 
> > > I don't view it as this way.  I think explicit should be the default for all
> > > ctors, and you need to have a good reason to have a ctor be |implicit|.
> > 
> > sure, if you change the language to make the default be explicit then an
> > explicit implicit marking is silly, but the language isn't that way so saying
> > this is explicit but actually can't ever be used for anything is just a waste
> > of space and reading time.
> 
> I'm not sure what you're suggesting here, Trevor.  Are you saying that you
> would like the analysis to magically ignore some constructors just because a
> human reader of the codee understands that the ctor cannot be abused?  I
> have no idea how to write an analysis that detects this.

no, I'm saying if you have a private unimplemented ctor it should be marked as deleted instead of explicit, which is something for the human fixing up all the ctors to do.

> > > > Maybe we should have the MFBT equivalent of chromium's
> > > > DISALLOW_EVIL_CONSTRUCTORS?  Then we can just stick the |explicit| in that
> > > > macro and be done with it.  And we can throw in things like MOZ_DELETE, too,
> > > > which none of these cases use (I realize these are older codes...).
> > > 
> > > Sure, that WFM but I'm not volunteering.  ;-)
> > 
> > How about having the analysis ignore deleted ctors, and then just marking these
> > as deleted? then leave the macro for someone else.
> 
> Like I said, the analysis already does ignore deleted ctors.  But please
> remember that you're asking me to investigate two things per tens of
> thousands of constructors in our code base, just to make the |explicit|
> keyword replaced by |MOZ_DELETE|.  That doesn't seem worth the effort to me.

presumably your adding explicit manually? it doesn't seem like that much extra work to me to guess and then check a particular ctor should be deleted.  Besides even if you make them explicit marking them deleted is a useful thing to do.

Comment 10

[(no longer active)]

(In reply to comment #9)
> > > > > Maybe we should have the MFBT equivalent of chromium's
> > > > > DISALLOW_EVIL_CONSTRUCTORS?  Then we can just stick the |explicit| in that
> > > > > macro and be done with it.  And we can throw in things like MOZ_DELETE, too,
> > > > > which none of these cases use (I realize these are older codes...).
> > > > 
> > > > Sure, that WFM but I'm not volunteering.  ;-)
> > > 
> > > How about having the analysis ignore deleted ctors, and then just marking these
> > > as deleted? then leave the macro for someone else.
> > 
> > Like I said, the analysis already does ignore deleted ctors.  But please
> > remember that you're asking me to investigate two things per tens of
> > thousands of constructors in our code base, just to make the |explicit|
> > keyword replaced by |MOZ_DELETE|.  That doesn't seem worth the effort to me.
> 
> presumably your adding explicit manually? it doesn't seem like that much extra
> work to me to guess and then check a particular ctor should be deleted. 
> Besides even if you make them explicit marking them deleted is a useful thing
> to do.

The thing is, it's actually quite non-obvious whether I can make a ctor as deleted, whereas with explicit the answer is almost yes.  To see if I can mark something as deleted, I need to find out whether the ctor is implemented, and that whether it's called by the code inside the class (assuming we're only talking about private ctors.)  That is quite some work if you multiply that by the number of ctors I need to look at.

But that being said, if I encounted a ctor in the future which is obviously unimplemented, I'll mark it as deleted instead.  No promises that I'll try this for every one of the ctors I'm going to examine though.  :-)

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #10)
> (In reply to comment #9)
> > > > > > Maybe we should have the MFBT equivalent of chromium's
> > > > > > DISALLOW_EVIL_CONSTRUCTORS?  Then we can just stick the |explicit| in that
> > > > > > macro and be done with it.  And we can throw in things like MOZ_DELETE, too,
> > > > > > which none of these cases use (I realize these are older codes...).
> > > > > 
> > > > > Sure, that WFM but I'm not volunteering.  ;-)
> > > > 
> > > > How about having the analysis ignore deleted ctors, and then just marking these
> > > > as deleted? then leave the macro for someone else.
> > > 
> > > Like I said, the analysis already does ignore deleted ctors.  But please
> > > remember that you're asking me to investigate two things per tens of
> > > thousands of constructors in our code base, just to make the |explicit|
> > > keyword replaced by |MOZ_DELETE|.  That doesn't seem worth the effort to me.
> > 
> > presumably your adding explicit manually? it doesn't seem like that much extra
> > work to me to guess and then check a particular ctor should be deleted. 
> > Besides even if you make them explicit marking them deleted is a useful thing
> > to do.
> 
> The thing is, it's actually quite non-obvious whether I can make a ctor as
> deleted, whereas with explicit the answer is almost yes.  To see if I can
> mark something as deleted, I need to find out whether the ctor is
> implemented, and that whether it's called by the code inside the class
> (assuming we're only talking about private ctors.)  That is quite some work
> if you multiply that by the number of ctors I need to look at.

I'd asume that any ctor declaration that isn't private shouldn't be marked deleted.  So the only nontrivial case is a private ctor with possible out of line implementation.  I'd tend to assume those should be deleted and let the compiler yell at me when they actually shouldn't be on the theory there pretty rarely anything else, but maybe I'm wrong and we have a bunch of those.

> But that being said, if I encounted a ctor in the future which is obviously
> unimplemented, I'll mark it as deleted instead.  No promises that I'll try
> this for every one of the ctors I'm going to examine though.  :-)

I'm not suggesting going crazy looking for things to mark deleted ;)

Comment 12

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1b430ecb3f5

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d1b430ecb3f5