Closed Bug 1009631 (explicit) Opened 6 years ago Closed 5 years ago

Detect one argument constructors which are not marked as explicit

Categories

(Firefox Build System :: Source Code Analysis, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla37

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file, 2 obsolete files)

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?
SGTM.
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...
Attachment #8425896 - Flags: review?(Pidgeot18)
Depends on: 1013662
Depends on: 1013663
Depends on: 1013664
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.
(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 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.
Attachment #8425896 - Flags: review?(Pidgeot18) → feedback+
Depends on: 1015012
Alias: explicit
Depends on: 1015430
Depends on: 1015663
Depends on: 1044596
Depends on: 1044598
Depends on: 1045041
Depends on: 1045065
Depends on: 1045067
Depends on: 1045068
Depends on: 1045091
Depends on: 1045092
Depends on: 1045094
Depends on: 1045436
Depends on: 1047778
Depends on: 1047781
Depends on: 1047782
Depends on: 1048013
Depends on: 1048239
Depends on: 1048240
Depends on: 1048241
Depends on: 1048242
Depends on: 1048243
Depends on: 1048245
Depends on: 1048246
Depends on: 1048247
Depends on: 1048252
Depends on: 1048271
Depends on: 1048272
Depends on: 1048280
Depends on: 1050609
Depends on: 1050610
Depends on: 1050611
Depends on: 1053603
Depends on: 1053605
Depends on: 1053792
Depends on: 1055001
Depends on: 1055517
Depends on: 1055519
Depends on: 1058169
Depends on: 1060375
Depends on: 1060802
Depends on: 1060930
Depends on: 1060973
Depends on: 1060974
Depends on: 1060975
Depends on: 1060976
Depends on: 1060977
Depends on: 1060978
Depends on: 1060980
Depends on: 1060982
Depends on: 1060983
Depends on: 1060985
Depends on: 1060986
Depends on: 1060987
Depends on: 1060988
Depends on: 1060989
Depends on: 1060991
Depends on: 1060993
Depends on: 1060994
Depends on: 1060997
Depends on: 1060999
Depends on: 1061000
Depends on: 1061001
Depends on: 1061002
Depends on: 1061009
Depends on: 1061023
Depends on: 1061047
Depends on: 1061048
Depends on: 1061049
Depends on: 1061050
Depends on: 1061051
Depends on: 1061052
Depends on: 1061053
Depends on: 1061054
Depends on: 1061056
Depends on: 1061057
Depends on: 1061058
Depends on: 1061059
Depends on: 1061060
Depends on: 1061061
Attachment #8425896 - Attachment is obsolete: true
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.
Attachment #8482081 - Flags: review?(Pidgeot18)
Depends on: 1061248
Depends on: 1061250
Depends on: 1061251
Depends on: 1061252
Depends on: 1061253
Depends on: 1061254
Attachment #8482081 - Attachment is obsolete: true
Attachment #8482081 - Flags: review?(Pidgeot18)
Attachment #8482328 - Flags: review?(Pidgeot18)
Depends on: 1062071
Depends on: 1062073
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!
Depends on: 1063086
(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?
Depends on: 1064354
Depends on: 1064356
Depends on: 1065664
Depends on: 1065667
Depends on: 1065668
Depends on: 1065670
Depends on: 1068020
Depends on: 1068022
Depends on: 1068024
Depends on: 1071571
Depends on: 1071573
Depends on: 1071575
(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.
(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.
Flags: needinfo?(Pidgeot18)
(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 :-)
Flags: needinfo?(Pidgeot18)
Depends on: 1079317
Depends on: 1079319
Depends on: 1079320
Depends on: 1079321
Depends on: 1079324
Depends on: 1079325
Depends on: 1079328
(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.  :-)
Flags: needinfo?(Pidgeot18)
Depends on: 1087306
Depends on: 1087307
Depends on: 1087308
Depends on: 1090242
Flags: needinfo?(Pidgeot18)
Attachment #8482328 - Flags: review?(Pidgeot18) → review+
Depends on: 1108595
Depends on: 1109246
Depends on: 1109694
Depends on: 1109697
Depends on: 1109699
Depends on: 1109702
Depends on: 1109705
Depends on: 1109729
Depends on: 1109746
https://hg.mozilla.org/mozilla-central/rev/30a3399193a8
https://hg.mozilla.org/mozilla-central/rev/1c0a42855a50
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Depends on: 1123429
Depends on: 1123459
Depends on: 1162766
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.