Depend on operator overload synthesis for three-way and equality comparisons. #174892

chinmaygarde · 2025-09-03T18:12:36Z

This depends on the implicitly generated operator!= from operator== in a few cases (see the exceptions below) and add operator<=> in some of the more obvious instances.

I've tried to be extremely conservative in this first pass. There are a few more candidates that were not converted because:

operator!= was different from !operator=. For instance, if it had early returns. Not sure how the compiler generated those and didn't want the behavior or performance characteristics to change.
They were virtual.
operator== didn't actually compare all struct members. Dubious but I didn't want to change existing behavior.
There were template parameters.

flutter-dashboard · 2025-09-03T18:12:40Z

It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

gemini-code-assist

Code Review

This pull request modernizes the C++ code by leveraging C++20's operator synthesis. It correctly removes redundant operator!= implementations, relying on the compiler to generate them from operator==. Additionally, it replaces groups of comparison operators with the three-way comparison operator (<=>), which simplifies the code. The changes are well-executed and improve code conciseness. I have one suggestion to further improve the code by moving a defaulted operator definition to the header for better clarity and potential compiler optimizations.

gemini-code-assist · 2025-09-03T18:14:41Z

engine/src/flutter/txt/src/txt/text_shadow.h


  TextShadow(SkColor color, SkPoint offset, double blur_sigma);

  bool operator==(const TextShadow& other) const;


For consistency and potential performance benefits, it's better to default the operator== directly in the header file. This allows the compiler to inline it more easily and makes the class definition more self-contained.¹

Suggested change

bool operator==(const TextShadow& other) const;

bool operator==(const TextShadow& other) const = default;

Style Guide References

Footnotes

The Google C++ Style Guide, which this project follows, recommends defaulting special member functions in the header, within the class definition, for clarity and potential inlining. ↩

Decent suggestion but I wanted to keep the behavior the same as much as possible.

gemini-code-assist · 2025-09-03T18:14:41Z

engine/src/flutter/txt/src/txt/text_shadow.cc

-bool TextShadow::operator!=(const TextShadow& other) const {
-  return !(*this == other);
-}
+bool TextShadow::operator==(const TextShadow& other) const = default;


If operator== is defaulted in the header as suggested for text_shadow.h, this out-of-line definition can be removed.

Ditto about being conservative and leaving the existing behavior be.

engine/src/flutter/display_list/display_list.h

engine/src/flutter/impeller/geometry/scalar.h

flar · 2025-09-03T19:17:00Z

engine/src/flutter/impeller/base/mask.h

-  constexpr bool operator!=(const Mask<EnumType>& other) const {
-    return mask_ != other.mask_;
-  }
+  constexpr auto operator<=>(const Mask<EnumType>& other) const = default;


Do any operators other than == make sense for a mask? That assumes a lot about the values of the enum and how they relate.

The bitwise operators (which make more sense) are also overloaded. This is just keeping the existing behavior consistent.

flar · 2025-09-03T19:19:08Z

engine/src/flutter/impeller/base/version.h

+    return *this >= other;
  }

+  constexpr auto operator<=>(const Version&) const = default;


Maybe add a comment to where the fields are defined that their order matters so that the default comparison operator does the right thing?

That's just the default behavior. Do we want to restate that everytime? Fine either way.

flar · 2025-09-03T19:22:23Z

engine/src/flutter/impeller/core/texture_descriptor.h

-  constexpr bool operator!=(const TextureDescriptor& other) const {
-    return !(*this == other);
-  }
+  constexpr bool operator==(const TextureDescriptor& other) const = default;


These default implementations bother me a little. They assume that all fields matter. What if someone adds a new field whose value is not relevant to comparison? This could create a maintenance issue in the future. I would feel happier if the default would take a list of fields to use.

On the other hand, with the old definition there was a potential maintenance issue that someone might add a new field that does matter in comparisons.

6 of one, half a fortnight of the other...

Yeah, there were a couple of instances in our codebase where not all were compared. I'm fine assuming everything matters. If it doesn't, we can be explicit about it and mention that in a comment.

The issue is that the fields are usually defined far from where the operators are defined and so there is no visual connection from one to the other that points out their inherent inter-dependency.

flar · 2025-09-03T19:23:22Z

engine/src/flutter/impeller/geometry/rational.cc

  }
 }

-bool Rational::operator!=(const Rational& that) const {


Should this have spaceship? It defines < below...

That would also add >, >=, and <=. Besides, the implementation seemed to do custom things. I wanted to keep the behavior the same in this patch. But if you want to add it now, we should totally go for it.

It would take some care. It would be outside the scope of this PR I think. Maybe file an issue?

Does spaceship have a "NaN" return value? I guess since it can return any comparable value, it could return a scalar and that could be NaN...

This would come up with denominators of 0.

BTW, it occurs to me that the implementation of these methods doesn't deal with 0 denominators anyway?

flar

Left a bunch of questions and suggestions, but didn't see anything wrong.

auto-submit · 2025-09-03T19:28:30Z

autosubmit label was removed for flutter/flutter/174892, because - The status or check suite Mac tool_integration_tests_5_5 has failed. Please fix the issues identified (or deflake) before re-applying this label.

The status or check suite Linux tool_integration_tests_3_7 has failed. Please fix the issues identified (or deflake) before re-applying this label.

chinmaygarde · 2025-09-03T19:31:24Z

Hmm, the test failures are related to fetching canvaskit from a CDN it seems:

Failed to fetch dynamically imported module: https://www.gstatic.com/flutter-canvaskit/bbd8d03cbcee1db909ac5d49d7cb51a49d92cd8d/chromium/canvaskit.js

jason-simmons · 2025-09-03T19:37:46Z

Hmm, the test failures are related to fetching canvaskit from a CDN it seems:

Failed to fetch dynamically imported module: https://www.gstatic.com/flutter-canvaskit/bbd8d03cbcee1db909ac5d49d7cb51a49d92cd8d/chromium/canvaskit.js

A fix is pending: #174891

…arisons. I've tried to be extremely conservative in this first pass. There are a few more candidates that were not converted because: * operator!= was different from !operator=. For instance, it had early returns. Not sure how the compiler generated those and didn't want the behavior or performance characteristics to change. * They were virtual. * operator== didn't actually compare all struct members. * There were template parameters.

auto-submit · 2025-09-03T22:17:28Z

autosubmit label was removed for flutter/flutter/174892, because - The status or check suite Windows windows_unopt has failed. Please fix the issues identified (or deflake) before re-applying this label.

…ality comparisons. (flutter/flutter#174892)

flutter/flutter@973320c...a082096 2025-09-09 [email protected] Roll Packages from 24588c6 to 2d651b2 (2 revisions) (flutter/flutter#175130) 2025-09-09 [email protected] Roll Skia from ab13fd19dd32 to 19ba56dde579 (2 revisions) (flutter/flutter#175127) 2025-09-09 [email protected] Roll Skia from 29a015f8712b to ab13fd19dd32 (2 revisions) (flutter/flutter#175108) 2025-09-09 [email protected] Roll Skia from 6a4613b83365 to 29a015f8712b (5 revisions) (flutter/flutter#175106) 2025-09-09 [email protected] Roll Dart SDK from 7b645442db0f to f446144fb7c9 (2 revisions) (flutter/flutter#175104) 2025-09-08 [email protected] Nav bar static components respect ambient MediaQueryData (flutter/flutter#174673) 2025-09-08 [email protected] fix typo in test documentation #2 (flutter/flutter#174707) 2025-09-08 [email protected] Update ImageReaderSurfaceProducer.MAX_IMAGES to include the maximum number of retained dequeued images (flutter/flutter#174971) 2025-09-08 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 5.0.0 to 6.0.1 in the all-github-actions group (flutter/flutter#175093) 2025-09-08 [email protected] Roll Skia from 25f00cb247f2 to 6a4613b83365 (3 revisions) (flutter/flutter#175087) 2025-09-08 [email protected] Roll Dart SDK from 83c6b6124380 to 7b645442db0f (1 revision) (flutter/flutter#175086) 2025-09-08 [email protected] Impeller: Convert GLProc name field and GLErrorToString to std::string_view (flutter/flutter#173771) 2025-09-08 [email protected] Depend on operator overload synthesis for three-way and equality comparisons. (flutter/flutter#174892) 2025-09-08 [email protected] Define a concept for UniqueObjectTraits. (flutter/flutter#174905) 2025-09-08 [email protected] Roll Skia from 0c2b0a00b7b5 to 25f00cb247f2 (1 revision) (flutter/flutter#175077) 2025-09-08 [email protected] Fix GitHub labeler platform-android typo (flutter/flutter#175076) 2025-09-08 [email protected] [Gradle 9] Removed `minSdkVersion` and only use `minSdk` (flutter/flutter#173892) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…arisons. (flutter#174892) This depends on the implicitly generated `operator!=` from `operator==` in a few cases (see the exceptions below) and add `operator<=>` in some of the more obvious instances. I've tried to be extremely conservative in this first pass. There are a few more candidates that were not converted because: * `operator!=` was different from `!operator=.` For instance, if it had early returns. Not sure how the compiler generated those and didn't want the behavior or performance characteristics to change. * They were virtual. * `operator==` didn't actually compare all struct members. Dubious but I didn't want to change existing behavior. * There were template parameters.

…r#9983) flutter/flutter@973320c...a082096 2025-09-09 [email protected] Roll Packages from 24588c6 to 2d651b2 (2 revisions) (flutter/flutter#175130) 2025-09-09 [email protected] Roll Skia from ab13fd19dd32 to 19ba56dde579 (2 revisions) (flutter/flutter#175127) 2025-09-09 [email protected] Roll Skia from 29a015f8712b to ab13fd19dd32 (2 revisions) (flutter/flutter#175108) 2025-09-09 [email protected] Roll Skia from 6a4613b83365 to 29a015f8712b (5 revisions) (flutter/flutter#175106) 2025-09-09 [email protected] Roll Dart SDK from 7b645442db0f to f446144fb7c9 (2 revisions) (flutter/flutter#175104) 2025-09-08 [email protected] Nav bar static components respect ambient MediaQueryData (flutter/flutter#174673) 2025-09-08 [email protected] fix typo in test documentation flutter#2 (flutter/flutter#174707) 2025-09-08 [email protected] Update ImageReaderSurfaceProducer.MAX_IMAGES to include the maximum number of retained dequeued images (flutter/flutter#174971) 2025-09-08 49699333+dependabot[bot]@users.noreply.github.com Bump actions/labeler from 5.0.0 to 6.0.1 in the all-github-actions group (flutter/flutter#175093) 2025-09-08 [email protected] Roll Skia from 25f00cb247f2 to 6a4613b83365 (3 revisions) (flutter/flutter#175087) 2025-09-08 [email protected] Roll Dart SDK from 83c6b6124380 to 7b645442db0f (1 revision) (flutter/flutter#175086) 2025-09-08 [email protected] Impeller: Convert GLProc name field and GLErrorToString to std::string_view (flutter/flutter#173771) 2025-09-08 [email protected] Depend on operator overload synthesis for three-way and equality comparisons. (flutter/flutter#174892) 2025-09-08 [email protected] Define a concept for UniqueObjectTraits. (flutter/flutter#174905) 2025-09-08 [email protected] Roll Skia from 0c2b0a00b7b5 to 25f00cb247f2 (1 revision) (flutter/flutter#175077) 2025-09-08 [email protected] Fix GitHub labeler platform-android typo (flutter/flutter#175076) 2025-09-08 [email protected] [Gradle 9] Removed `minSdkVersion` and only use `minSdk` (flutter/flutter#173892) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…ality comparisons. (flutter/flutter#174892)

…arisons. (flutter#174892) This depends on the implicitly generated `operator!=` from `operator==` in a few cases (see the exceptions below) and add `operator<=>` in some of the more obvious instances. I've tried to be extremely conservative in this first pass. There are a few more candidates that were not converted because: * `operator!=` was different from `!operator=.` For instance, if it had early returns. Not sure how the compiler generated those and didn't want the behavior or performance characteristics to change. * They were virtual. * `operator==` didn't actually compare all struct members. Dubious but I didn't want to change existing behavior. * There were template parameters.

chinmaygarde requested review from flar, gaaclarke and jason-simmons September 3, 2025 18:12

github-actions bot added a: text input Entering text in a text field or keyboard related problems engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Sep 3, 2025

gemini-code-assist bot reviewed Sep 3, 2025

View reviewed changes

jason-simmons approved these changes Sep 3, 2025

View reviewed changes

chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 3, 2025

gaaclarke reviewed Sep 3, 2025

View reviewed changes

engine/src/flutter/display_list/display_list.h Show resolved Hide resolved

chinmaygarde commented Sep 3, 2025

View reviewed changes

engine/src/flutter/impeller/geometry/scalar.h Show resolved Hide resolved

flar reviewed Sep 3, 2025

View reviewed changes

flar approved these changes Sep 3, 2025

View reviewed changes

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 3, 2025

chinmaygarde force-pushed the defaultoper branch from bbd8d03 to 680d90b Compare September 3, 2025 21:19

chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 3, 2025

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 3, 2025

chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2025

auto-submit bot added this pull request to the merge queue Sep 8, 2025

Merged via the queue into flutter:master with commit df8e02d Sep 8, 2025
185 checks passed

flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Sep 8, 2025

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2025

df8e02d5b Depend on operator overload synthesis for three-way and equ…

95cb27c

…ality comparisons. (flutter/flutter#174892)

engine-flutter-autoroll mentioned this pull request Sep 9, 2025

Roll Flutter from 973320c0270b to 4277a18e139b (15 revisions) flutter/packages#9982

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2025

df8e02d5b Depend on operator overload synthesis for three-way and equ…

3d5e867

…ality comparisons. (flutter/flutter#174892)

engine-flutter-autoroll mentioned this pull request Sep 9, 2025

Roll Flutter from 973320c0270b to a0820964d59c (17 revisions) flutter/packages#9983

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2025

df8e02d5b Depend on operator overload synthesis for three-way and equ…

f864533

…ality comparisons. (flutter/flutter#174892)

engine-flutter-autoroll mentioned this pull request Sep 9, 2025

Roll Flutter from 973320c0270b to a0820964d59c (17 revisions) flutter/packages#9984

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 9, 2025

df8e02d5b Depend on operator overload synthesis for three-way and equ…

3115b16

…ality comparisons. (flutter/flutter#174892)

engine-flutter-autoroll mentioned this pull request Sep 9, 2025

Roll Flutter from 973320c0270b to 53f8432312ed (18 revisions) flutter/packages#9985

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 12, 2025

df8e02d5b Depend on operator overload synthesis for three-way and equ…

541ade5

…ality comparisons. (flutter/flutter#174892)


		TextShadow(SkColor color, SkPoint offset, double blur_sigma);

		bool operator==(const TextShadow& other) const;

Depend on operator overload synthesis for three-way and equality comparisons. #174892

Depend on operator overload synthesis for three-way and equality comparisons. #174892

Uh oh!

Conversation

chinmaygarde commented Sep 3, 2025

Uh oh!

flutter-dashboard bot commented Sep 3, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 3, 2025

Choose a reason for hiding this comment

Style Guide References

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Sep 3, 2025

Uh oh!

chinmaygarde commented Sep 3, 2025

Uh oh!

jason-simmons commented Sep 3, 2025

Uh oh!

auto-submit bot commented Sep 3, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants