1120059 - Remove now unnecessary MOZ_EXPLICIT_CONVERSION macro

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Biru [:poiru]]

Explicit operator conversion is now supported by all of our compilers.

Comment 1

[Biru [:poiru]]

Attachment: Remove MOZ_{HAVE_,}EXPLICIT_CONVERSION

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8547211 [details] [diff] [review]
Remove MOZ_{HAVE_,}EXPLICIT_CONVERSION

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

Fine as far as it goes, but we should go the next step and start putting explicit operator bool() (at least) to use in places where we did our best at hackarounds previously.  The hits for "convertibletobool" in the tree more or less all need changing, except for the security/ hit.

Comment 3

[Biru [:poiru]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11aebde6e809

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> Fine as far as it goes, but we should go the next step and start putting
> explicit operator bool() (at least) to use in places where we did our best
> at hackarounds previously.  The hits for "convertibletobool" in the tree
> more or less all need changing, except for the security/ hit.

Filed bug 1120059 for that.

Comment 4

[Biru [:poiru]]

(In reply to Birunthan Mohanathas [:poiru] from comment #3)
> Filed bug 1120059 for that.

Bug 1120796, that is.

Comment 5

[(no longer active)]

This broke Windows builds:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7e143e1f0852

So I backed it out: <https://hg.mozilla.org/integration/mozilla-inbound/rev/5f146f3b1362>

(Ignore the errors in TabParent.cpp, that's not relevant.)

Comment 6

[Biru [:poiru]]

VS2013 doesn't seem to like templated explicit conversion operators. It looks like the issue has been fixed for VS2015 (based on quick test with http://webcompiler.cloudapp.net/).

Waldo, should I just add #ifdefs to check for VS2015 in the relevant places or would you prefer something general like MOZ_TEMPLATED_EXPLICIT_CONVERSION?

Comment 7

[Jeff Walden [:Waldo]]

Add something general (or morph the current thing into it).  Sigh.  Oh well.

Comment 8

[Biru [:poiru]]

Attachment: Remove MOZ_{HAVE_,}EXPLICIT_CONVERSION

Comment 9

[Masatoshi Kimura [:emk]]

Comment on attachment 8549035 [details] [diff] [review]
Remove MOZ_{HAVE_,}EXPLICIT_CONVERSION

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

::: mfbt/Attributes.h
@@ -60,5 @@
>  #      define MOZ_HAVE_CXX11_CONSTEXPR
>  #    endif
> -#    if __has_extension(cxx_explicit_conversions)
> -#      define MOZ_HAVE_EXPLICIT_CONVERSION
> -#    endif

Doesn't clang-cl support templated explicit conversions?

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8549035 [details] [diff] [review]
Remove MOZ_{HAVE_,}EXPLICIT_CONVERSION

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

::: mfbt/Attributes.h
@@ -60,5 @@
>  #      define MOZ_HAVE_CXX11_CONSTEXPR
>  #    endif
> -#    if __has_extension(cxx_explicit_conversions)
> -#      define MOZ_HAVE_EXPLICIT_CONVERSION
> -#    endif

Yeah, I suspect this should be replaced with #      define MOZ_HAVE_TEMPLATED_EXPLICIT_CONVERSION.

@@ +146,2 @@
>   *     }
>   *   };

Given I don't understand whether it's templated explicit operators entirely, or just anywhere the type variable is the cast type, that doesn't work with MSVC (see the comment on the widget/ file): please clarify here exactly what it is that requires this.

::: widget/BasicEvents.h
@@ +883,5 @@
>        return !mBuffer.IsEmpty();
>      }
>  
>      template<typename T>
> +    explicit operator const T*() const

...I thought MSVC didn't have support for this, and this needed to be MOZ_TEMPLATED_EXPLICIT_CONVERSION.  Or is it only |template<typename T> explicit operator T()| that fails, and T* and const T* don't?

Comment 11

[Biru [:poiru]]

Probably better to wait until we use VS2015.

Comment 12

[Chris Peterson [:cpeterson]]

Birunthan, we just dropped support for VS2013 (in bug 1186064), so all our officially-supported compilers now support explicit conversion operators. Do you have time to continue working on your patch?

Comment 13

[Michelangelo De Simone [Offline] [:mds]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26839a24ba97

Comment 14

[Michelangelo De Simone [Offline] [:mds]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a65ff0ddaa6

Comment 15

[Biru [:poiru]]

(In reply to Chris Peterson [:cpeterson] from comment #12)
> Birunthan, we just dropped support for VS2013 (in bug 1186064), so all our
> officially-supported compilers now support explicit conversion operators. Do
> you have time to continue working on your patch?

Seems like Michelangelo is working on this so clearing ni?.

Comment 16

[Michelangelo De Simone [Offline] [:mds]]

(In reply to Birunthan Mohanathas [:poiru] from comment #15)

> Seems like Michelangelo is working on this so clearing ni?.

Ouch, I didn't know the try-server would have spoiled my evil plan to take this over.:)

Comment 17

[Michelangelo De Simone [Offline] [:mds]]

Attachment: Bug 1120059 - Removing unnecessary MOZ_EXPLICIT_CONVERSION macros.

Review commit: https://reviewboard.mozilla.org/r/64382/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64382/

Comment 18

[Chris Peterson [:cpeterson]]

Michelangelo, are you still working on this patch to the MOZ_EXPLICIT_CONVERSION macro? Your patch summary says "r?jwalden" but I don't think you actually flagged Waldo for review.

Also, there were some build failures on the try push from MozReview, though they don't really look related to your changes:

https://reviewboard.mozilla.org/r/64382/diff/1#index_header
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8200b099f7dc&selectedJob=23918511

/home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Logging.h:143:22: error: constexpr constructor never produces a constant expression [-Winvalid-constexpr]
/home/worker/workspace/build/src/xpcom/glue/PLDHashTable.h:78:13: error: constexpr constructor never produces a constant expression [-Winvalid-constexpr]
/home/worker/workspace/build/src/MacOSX10.7.sdk/usr/include/c++/v1/atomic:569:32: error: cannot initialize a parameter of type 'unsigned long *' with an lvalue of type 'unsigned long'
/home/worker/workspace/build/src/MacOSX10.7.sdk/usr/include/c++/v1/atomic:575:41: error: too few arguments to function call, expected 3, have 2
/home/worker/workspace/build/src/MacOSX10.7.sdk/usr/include/c++/v1/atomic:575:41: error: too few arguments to function call, expected 3, have 2
/home/worker/workspace/build/src/MacOSX10.7.sdk/usr/include/c++/v1/atomic:569:32: error: cannot initialize a parameter of type 'mozilla::LogLevel *' with an lvalue of type 'mozilla::LogLevel'
/home/worker/workspace/build/src/MacOSX10.7.sdk/usr/include/c++/v1/atomic:575:41: error: too few arguments to function call, expected 3, have 2
/home/worker/workspace/build/src/MacOSX10.7.sdk/usr/include/c++/v1/atomic:569:32: error: cannot initialize a parameter of type 'mozilla::LogModule **' with an lvalue of type 'mozilla::LogModule *'

Comment 19

[Michelangelo De Simone [Offline] [:mds]]

(In reply to Chris Peterson [:cpeterson] from comment #18)

> Michelangelo, are you still working on this patch to the
> MOZ_EXPLICIT_CONVERSION macro? Your patch summary says "r?jwalden" but I
> don't think you actually flagged Waldo for review.

Oops. Fixed, sorry for that.

> Also, there were some build failures on the try push from MozReview, though
> they don't really look related to your changes:

Yep, the Mac try-servers seem busted; I've triggered a new build, let's see what happens.

Comment 20

[Jeff Walden [:Waldo]]

Comment on attachment 8771136 [details]
Bug 1120059 - Removing unnecessary MOZ_EXPLICIT_CONVERSION macros.

https://reviewboard.mozilla.org/r/64382/#review72954

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dc04679e7f2
Remove unnecessary MOZ_EXPLICIT_CONVERSION macros. r=jwalden

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7dc04679e7f2