1009631 - (explicit) Detect one argument constructors which are not marked as explicit

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[(no longer active)]

My current idea is to error out when finding such constructors unless if they are copy constructors or have an attribute hidden behind MOZ_IMPLICIT or something like that.

Thoughts?

Comment 1

[Benjamin Smedberg]

SGTM.

Comment 2

[(no longer active)]

Attachment: Add a static check to the clang plugin to detect bad implicit conversion constructors; r=jcranmer

Comment 3

[(no longer active)]

Comment on attachment 8425896 [details] [diff] [review]
Add a static check to the clang plugin to detect bad implicit conversion constructors; r=jcranmer

I'll fix Gecko to actually stick to these rules in the many dependencies which I will file as I go on.  I may need to whitelist more stuff that just namespace std and icu_52 in the future as needed, but I hope to not have to do that...

Comment 4

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8425896 [details] [diff] [review]
Add a static check to the clang plugin to detect bad implicit conversion constructors; r=jcranmer

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

::: build/clang-plugin/clang-plugin.cpp
@@ +48,5 @@
>  };
>  
> +namespace {
> +
> +bool isInIgnoredNamespace(const Decl *decl) {

I think the goal you want here is not whether or not it is in an ignored namespace but whether or not the declaration is an interesting in. So name this something like "isInterestingDecl".

I'm also distrustful of using namespaces to resolve declarations, particularly since this is likely to interact poorly with C++ widget APIs we use (like say QT). I'm not sure off-hand what the best indication to use here is, though.

Comment 5

[(no longer active)]

(In reply to Joshua Cranmer [:jcranmer] from comment #4)
> Comment on attachment 8425896 [details] [diff] [review]
> Add a static check to the clang plugin to detect bad implicit conversion
> constructors; r=jcranmer
> 
> Review of attachment 8425896 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/clang-plugin/clang-plugin.cpp
> @@ +48,5 @@
> >  };
> >  
> > +namespace {
> > +
> > +bool isInIgnoredNamespace(const Decl *decl) {
> 
> I think the goal you want here is not whether or not it is in an ignored
> namespace but whether or not the declaration is an interesting in. So name
> this something like "isInterestingDecl".

Sure.

> I'm also distrustful of using namespaces to resolve declarations,
> particularly since this is likely to interact poorly with C++ widget APIs we
> use (like say QT). I'm not sure off-hand what the best indication to use
> here is, though.

Well, I'm open to other suggestions, but I don't have better ideas myself either.  Perhaps we can hold off on this until I have more of the tree build with this analysis to see what other interesting cases I'll find?

Comment 6

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8425896 [details] [diff] [review]
Add a static check to the clang plugin to detect bad implicit conversion constructors; r=jcranmer

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

Yeah, I think waiting until conversion is further along and then revisiting this is the better way to go for now.

Comment 7

[(no longer active)]

Attachment: Add a static check to the clang plugin to detect bad implicit conversion constructors; r=jcranmer

Comment 8

[(no longer active)]

Comment on attachment 8482081 [details] [diff] [review]
Add a static check to the clang plugin to detect bad implicit conversion constructors; r=jcranmer

OK, I had to exclude code from these checks based on namespaces and directory names in the full path to the source file.  I think this is a pragmatic way of ensuring that we don't need to modify code that we do not own here.

Comment 9

[(no longer active)]

Attachment: Add a static check to the clang plugin to detect bad implicit conversion constructors; r=jcranmer

Comment 10

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8482328 [details] [diff] [review]
Add a static check to the clang plugin to detect bad implicit conversion constructors; r=jcranmer

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

::: build/clang-plugin/clang-plugin.cpp
@@ +77,5 @@
> +         ND->getName() == "stagefright" ||      // libstagefright
> +         ND->getName() == "MacFileUtilities" || // MacFileUtilities
> +         ND->getName() == "dwarf2reader" ||     // dwarf2reader
> +         ND->getName() == "arm_ex_to_module" || // arm_ex_to_module
> +         ND->getName() == "testing";            // gtest

Might it be easier to instead only check for namespaces in Mozilla namespaces? IIRC, we mostly use mozilla::*, js::* (JS::*?), but I don't know my way around most of m-c.

@@ +94,5 @@
> +        begin->compare_lower(StringRef("angle")) == 0 ||
> +        begin->compare_lower(StringRef("harfbuzz")) == 0 ||
> +        begin->compare_lower(StringRef("hunspell")) == 0 ||
> +        begin->compare_lower(StringRef("scoped_ptr.h")) == 0 ||
> +        begin->compare_lower(StringRef("graphite2")) == 0) {

I think in this case (and the previous case), it might be best to check for containment in a short vector. Since this code is using clang to be compiled in the first place, we could probably use C++11 and initializer lists to make a static std::array or std::vector. Less boilerplate is better!

Comment 11

[(no longer active)]

(In reply to Joshua Cranmer [:jcranmer] from comment #10)
> Comment on attachment 8482328 [details] [diff] [review]
> Add a static check to the clang plugin to detect bad implicit conversion
> constructors; r=jcranmer
> 
> Review of attachment 8482328 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: build/clang-plugin/clang-plugin.cpp
> @@ +77,5 @@
> > +         ND->getName() == "stagefright" ||      // libstagefright
> > +         ND->getName() == "MacFileUtilities" || // MacFileUtilities
> > +         ND->getName() == "dwarf2reader" ||     // dwarf2reader
> > +         ND->getName() == "arm_ex_to_module" || // arm_ex_to_module
> > +         ND->getName() == "testing";            // gtest
> 
> Might it be easier to instead only check for namespaces in Mozilla
> namespaces? IIRC, we mostly use mozilla::*, js::* (JS::*?), but I don't know
> my way around most of m-c.

Unfortunately we have a _ton_ of code in the global namespace, so I don't think this is a good idea.

> @@ +94,5 @@
> > +        begin->compare_lower(StringRef("angle")) == 0 ||
> > +        begin->compare_lower(StringRef("harfbuzz")) == 0 ||
> > +        begin->compare_lower(StringRef("hunspell")) == 0 ||
> > +        begin->compare_lower(StringRef("scoped_ptr.h")) == 0 ||
> > +        begin->compare_lower(StringRef("graphite2")) == 0) {
> 
> I think in this case (and the previous case), it might be best to check for
> containment in a short vector. Since this code is using clang to be compiled
> in the first place, we could probably use C++11 and initializer lists to
> make a static std::array or std::vector. Less boilerplate is better!

I actually tried to do this.  The problem I ran into is that requires -stdlib=libc++ which is broken by default on OSX for local clang builds because of http://llvm.org/bugs/show_bug.cgi?id=20357.  I would really like to be able to tell people to just drop in --enable-clang-plugin in their local mozconfigs and get benefit of these checks in their local development without having to worry about these types of issues, so I'd like to avoid doing that.  What do you think?

Comment 12

[Joshua Cranmer [:jcranmer]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #11)
> (In reply to Joshua Cranmer [:jcranmer] from comment #10)
> > Might it be easier to instead only check for namespaces in Mozilla
> > namespaces? IIRC, we mostly use mozilla::*, js::* (JS::*?), but I don't know
> > my way around most of m-c.
> 
> Unfortunately we have a _ton_ of code in the global namespace, so I don't
> think this is a good idea.

The global namespace could be done in addition to the mozilla style namespaces, I think.

> I actually tried to do this.  The problem I ran into is that requires
> -stdlib=libc++ which is broken by default on OSX for local clang builds
> because of http://llvm.org/bugs/show_bug.cgi?id=20357.  I would really like
> to be able to tell people to just drop in --enable-clang-plugin in their
> local mozconfigs and get benefit of these checks in their local development
> without having to worry about these types of issues, so I'd like to avoid
> doing that.  What do you think?

I think we should be yelling more loudly to LLVM/Clang folks about getting this sort of stuff working. I won't block this review on fixing this issue, but please notate in the code why this is necessary and file a bug on fixing it when clang gets its act together.

Comment 13

[(no longer active)]

(In reply to Joshua Cranmer [:jcranmer] from comment #12)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #11)
> > (In reply to Joshua Cranmer [:jcranmer] from comment #10)
> > > Might it be easier to instead only check for namespaces in Mozilla
> > > namespaces? IIRC, we mostly use mozilla::*, js::* (JS::*?), but I don't know
> > > my way around most of m-c.
> > 
> > Unfortunately we have a _ton_ of code in the global namespace, so I don't
> > think this is a good idea.
> 
> The global namespace could be done in addition to the mozilla style
> namespaces, I think.

OK, but what about other random namespaces people might use?  I mean, doing what you're suggesting is a few less lines of code, but it will probably cause us to miss stuff.  Is there any good reason to not keep this opt out?

> > I actually tried to do this.  The problem I ran into is that requires
> > -stdlib=libc++ which is broken by default on OSX for local clang builds
> > because of http://llvm.org/bugs/show_bug.cgi?id=20357.  I would really like
> > to be able to tell people to just drop in --enable-clang-plugin in their
> > local mozconfigs and get benefit of these checks in their local development
> > without having to worry about these types of issues, so I'd like to avoid
> > doing that.  What do you think?
> 
> I think we should be yelling more loudly to LLVM/Clang folks about getting
> this sort of stuff working.

Well, there's that bug that is on file... If you know of a better way of yelling, I'm all ears. :)

> I won't block this review on fixing this issue,
> but please notate in the code why this is necessary and file a bug on fixing
> it when clang gets its act together.

Will do.

Comment 14

[Joshua Cranmer [:jcranmer]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #13)
> (In reply to Joshua Cranmer [:jcranmer] from comment #12)
> > The global namespace could be done in addition to the mozilla style
> > namespaces, I think.
> 
> OK, but what about other random namespaces people might use?  I mean, doing
> what you're suggesting is a few less lines of code, but it will probably
> cause us to miss stuff.  Is there any good reason to not keep this opt out?

I suppose in theory, namespace usage in first-party code should be sane and simple, but it's probably not so much in practice (I don't work enough in m-c to know). My main concern is that the opt-out approach requires us to include more lines every time we import third-party code, since third-party is unlikely to conform to our style guidelines.

> > I think we should be yelling more loudly to LLVM/Clang folks about getting
> > this sort of stuff working.
> 
> Well, there's that bug that is on file... If you know of a better way of
> yelling, I'm all ears. :)

Haranguing people at an LLVM Developers Meeting can be productive :-)

Comment 15

[(no longer active)]

(In reply to Joshua Cranmer [:jcranmer] from comment #14)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #13)
> > (In reply to Joshua Cranmer [:jcranmer] from comment #12)
> > > The global namespace could be done in addition to the mozilla style
> > > namespaces, I think.
> > 
> > OK, but what about other random namespaces people might use?  I mean, doing
> > what you're suggesting is a few less lines of code, but it will probably
> > cause us to miss stuff.  Is there any good reason to not keep this opt out?
> 
> I suppose in theory, namespace usage in first-party code should be sane and
> simple, but it's probably not so much in practice (I don't work enough in
> m-c to know).

That theory definitely doesn't hold for our code.

> My main concern is that the opt-out approach requires us to
> include more lines every time we import third-party code, since third-party
> is unlikely to conform to our style guidelines.

For sure, but why is that a problem?  Most of the code that we add is not third-party code, so I think that's a perfectly reasonable trade-off.

But if you still feel strongly about this, I can do what you're suggesting, and we will get much less coverage, but I guess that's better than getting no coverage at all by making this patch stay on my local tree. :/

> > > I think we should be yelling more loudly to LLVM/Clang folks about getting
> > > this sort of stuff working.
> > 
> > Well, there's that bug that is on file... If you know of a better way of
> > yelling, I'm all ears. :)
> 
> Haranguing people at an LLVM Developers Meeting can be productive :-)

Please do.  :-)

Comment 16

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/30a3399193a8

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/30a3399193a8
https://hg.mozilla.org/mozilla-central/rev/1c0a42855a50

Comment 18

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/mozilla-central/rev/294cfcf466c8