Workaround a release blocker after libc++ change #43091
Conversation
The code breaks in C++20 mode after libc++ removes comparisons for `std::vector` and replaces them with 'operator <=>'.
|
This pull request was opened against a branch other than main. Since Flutter pull requests should not normally be opened against branches other than main, I have changed the base to main. If this was intended, you may modify the base back to master. See the Release Process for information about how other branches get updated. Reviewers: Use caution before merging pull requests to branches other than main, unless this is an intentional hotfix/cherrypick. |
stuartmorgan-g
left a comment
stuartmorgan-g
left a comment
There was a problem hiding this comment.
What's the failure mode here? Ideally we should be testing this.
| return std::get<int64_t>(*this); | ||
| } | ||
|
|
||
| // Avoid operator<=> problems with std::variant in C++20 mode. |
There was a problem hiding this comment.
What are the problems? Even reading the internal change I'm not clear on what problem is being solved.
And is this permanent, or a temporary workaround? If the latter, what are the conditions for removing it?
There was a problem hiding this comment.
From internal change:
The problem is the following build breakage: [link redacted] See b/288103464 for details. Using variant as base class makes its operator <=> non-applicable in overloading because it will recursively check if the type itself has operator <=> (through variant vector<EncodedValue>)
The workaround is permanent. After a few releases of libc++, it would be probably be better to define operator <=> instead of operator <.
Note that operator <=> is C++20-only.
|
|
||
| // Avoid operator<=> problems with std::variant in C++20 mode. | ||
| friend bool operator<(const EncodableValue& lhs, const EncodableValue& rhs) { | ||
| return static_cast<const super&>(lhs) < static_cast<const super&>(rhs); |
There was a problem hiding this comment.
I'm not following this code. What does an explicit cast to a super reference do? This needs an explanatory comment.
There was a problem hiding this comment.
From internal change:
This code says "define < comparison of encodable values as the < comparison of the underlying variant". (I've added a comment saying this)
The cast is needed because lhs < rhs would just recursively call the operator< we're trying to define here. (I wouldn't normally add a comment to this, feel free to suggest one)
The high level idea is: expose an operator< directly on EncodableType so that vector<EncodableType> < vector<EncodableType> will delegate to that. Currently such comparisons fail to compile, for reasons that are pretty obscure.
My best understanding is that variant's/vector's operator<=> do not work properly when you inherit from variant/vector, and this is not a bug in libc++ but is specified in C++20. (I would further guess: this is not specifically intended, but an interaction of many factors and backwards-compat for code that inherits from stdlib classes is not a high priority. operator<=> is an outlier in that it breaks more existing code than most language changes).
In any case, most of this is out of our hands: the new compiler rejects this code, it appears to be correct in doing so, I don't have a simple intuitive explanation why. Happy to add whatever comments you like though, please suggest some text.
|
I don't have much context to be honest. I've copied your responses to the internal change and let's continue the discussion there. Thanks. |
|
Updated with replies from internal change, PTAL, thanks. |
|
I've added a suggested comment for the method internally; if everyone is fine with that text I think we're good. |
|
Updated the comments here according to the discussion internally. PTAL, thanks! |
chingjun
added
the
autosubmit
…129378) flutter/engine@703c9a1...ed477de 2023-06-22 [email protected] Roll ANGLE from bbcf54bcb738 to 4ed2d403a329 (7 revisions) (flutter/engine#43105) 2023-06-22 [email protected] Workaround a release blocker after libc++ change (flutter/engine#43091) 2023-06-22 [email protected] Roll Skia from 8168c802c391 to 09b36b8ce0db (1 revision) (flutter/engine#43102) 2023-06-22 [email protected] [Impeller] Reland: Correctly compute UVs in texture fill (flutter/engine#43093) 2023-06-22 [email protected] [Impeller] Add validation forbidding SamplerAddressMode::kDecal on the OpenGLES backend (flutter/engine#43094) 2023-06-22 [email protected] Use minor version, ignore patches for CodeQL (flutter/engine#43088) 2023-06-22 [email protected] Print a warning when a message channel is used on the wrong thread. (flutter/engine#42928) 2023-06-22 [email protected] Roll Skia from 3f3e1da4b7eb to 8168c802c391 (4 revisions) (flutter/engine#43096) 2023-06-22 [email protected] [Impeller] default sample count back to 1 (but configure to 4 in defaults). (flutter/engine#43089) 2023-06-22 [email protected] [web] Don't get break type from v8BreakIterator (flutter/engine#43053) 2023-06-22 [email protected] Roll dart to 3.1.0-239.0.dev (flutter/engine#43083) 2023-06-22 [email protected] Revert "[Impeller] dont use concurrent runner to decode images on Android." (flutter/engine#43061) 2023-06-22 [email protected] [Impeller] Add fence waiter trace event. (flutter/engine#43092) 2023-06-22 [email protected] [Impeller] remove Vulkan pipeline cache mutex. (flutter/engine#43085) 2023-06-22 [email protected] Revert "[Impeller] Correctly compute UVs in texture fill" (flutter/engine#43087) 2023-06-22 [email protected] Roll Fuchsia Linux SDK from 7EZeNE4aGd29VfDly... to tcVndpnH_jzGm5LsJ... (flutter/engine#43081) 2023-06-22 [email protected] Roll Skia from 117f57a53215 to 3f3e1da4b7eb (4 revisions) (flutter/engine#43080) 2023-06-22 [email protected] Roll ANGLE from 7658525166a4 to bbcf54bcb738 (1 revision) (flutter/engine#43079) 2023-06-22 [email protected] Roll Skia from 5013b651f8ec to 117f57a53215 (1 revision) (flutter/engine#43078) 2023-06-22 [email protected] Roll Fuchsia Mac SDK from QtQznuUmHMTyORqxJ... to Ylc35wOk0_j0NLzDv... (flutter/engine#43076) 2023-06-22 [email protected] Roll ANGLE from a2b3f9b64670 to 7658525166a4 (1 revision) (flutter/engine#43075) 2023-06-22 [email protected] Roll ANGLE from ac263582dda4 to a2b3f9b64670 (1 revision) (flutter/engine#43074) 2023-06-22 [email protected] Roll Skia from 71047dca9f77 to 5013b651f8ec (4 revisions) (flutter/engine#43073) 2023-06-22 [email protected] [Impeller] Correctly compute UVs in texture fill (flutter/engine#43028) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from 7EZeNE4aGd29 to tcVndpnH_jzG fuchsia/sdk/core/mac-amd64 from QtQznuUmHMTy to Ylc35wOk0_j0 If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
2 participants
The code breaks in C++20 mode after libc++ removes comparisons for
std::vectorand replaces them with 'operator <=>'.See cl/542541552 for context.