Closed
Bug 1120059
Opened 10 years ago
Closed 8 years ago
Remove now unnecessary MOZ_EXPLICIT_CONVERSION macro
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla51
People
(Reporter: poiru, Assigned: mds)
References
Details
Attachments
(1 file, 2 obsolete files)
Explicit operator conversion is now supported by all of our compilers.
Updated•10 years ago
|
Reporter | ||
Comment 1•10 years ago
|
||
Attachment #8547211 -
Flags: review?(jwalden+bmo)
Comment 2•10 years ago
|
||
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+
Reporter | ||
Comment 3•10 years ago
|
||
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.
Reporter | ||
Comment 4•10 years ago
|
||
(In reply to Birunthan Mohanathas [:poiru] from comment #3)
> Filed bug 1120059 for that.
Bug 1120796, that is.
Comment 5•10 years ago
|
||
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.)
Reporter | ||
Comment 6•10 years ago
|
||
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)
Comment 7•10 years ago
|
||
Add something general (or morph the current thing into it). Sigh. Oh well.
Flags: needinfo?(jwalden+bmo)
Reporter | ||
Comment 8•10 years ago
|
||
Attachment #8547211 -
Attachment is obsolete: true
Attachment #8549035 -
Flags: review?(jwalden+bmo)
Comment 9•10 years ago
|
||
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•10 years ago
|
||
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+
Reporter | ||
Comment 11•9 years ago
|
||
Probably better to wait until we use VS2015.
Assignee: birunthan → nobody
Status: ASSIGNED → NEW
Comment 12•8 years ago
|
||
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
status-firefox49:
--- → wontfix
status-firefox50:
--- → affected
Flags: needinfo?(birunthan)
Assignee | ||
Comment 13•8 years ago
|
||
Assignee | ||
Comment 14•8 years ago
|
||
Reporter | ||
Comment 15•8 years ago
|
||
(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)
Assignee | ||
Comment 16•8 years ago
|
||
(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.:)
Assignee | ||
Comment 17•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/64382/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64382/
Assignee | ||
Updated•8 years ago
|
Attachment #8549035 -
Attachment is obsolete: true
Comment 18•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #8771136 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 19•8 years ago
|
||
(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 20•8 years ago
|
||
mozreview-review |
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+
Updated•8 years ago
|
Assignee: nobody → michelangelo
Status: NEW → ASSIGNED
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 21•8 years ago
|
||
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
Comment 22•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in
before you can comment on or make changes to this bug.
Description
•