Closed Bug 1120059 Opened 5 years ago Closed 3 years ago

Remove now unnecessary MOZ_EXPLICIT_CONVERSION macro

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox49 --- wontfix
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: poiru, Assigned: mds)

References

Details

Attachments

(1 file, 2 obsolete files)

Explicit operator conversion is now supported by all of our compilers.
Attachment #8547211 - Flags: review?(jwalden+bmo)
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.
Attachment #8547211 - Flags: review?(jwalden+bmo) → review+
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.
(In reply to Birunthan Mohanathas [:poiru] from comment #3)
> Filed bug 1120059 for that.

Bug 1120796, that is.
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.)
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?
Flags: needinfo?(jwalden+bmo)
Add something general (or morph the current thing into it).  Sigh.  Oh well.
Flags: needinfo?(jwalden+bmo)
Attachment #8547211 - Attachment is obsolete: true
Attachment #8549035 - Flags: review?(jwalden+bmo)
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 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?
Attachment #8549035 - Flags: review?(jwalden+bmo) → feedback+
Probably better to wait until we use VS2015.
Assignee: birunthan → nobody
Status: ASSIGNED → NEW
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?
Blocks: 1080968
Depends on: 1186064
No longer depends on: 1117820
Flags: needinfo?(birunthan)
(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?.
Flags: needinfo?(birunthan)
(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.:)
Attachment #8549035 - Attachment is obsolete: true
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 *'
Flags: needinfo?(michelangelo)
Attachment #8771136 - Flags: review?(jwalden+bmo)
(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.
Flags: needinfo?(michelangelo)
Comment on attachment 8771136 [details]
Bug 1120059 - Removing unnecessary MOZ_EXPLICIT_CONVERSION macros.

https://reviewboard.mozilla.org/r/64382/#review72954
Attachment #8771136 - Flags: review?(jwalden+bmo) → review+
Assignee: nobody → michelangelo
Status: NEW → ASSIGNED
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dc04679e7f2
Remove unnecessary MOZ_EXPLICIT_CONVERSION macros. r=jwalden
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7dc04679e7f2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.