Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@sjindel-google
Copy link
Contributor

By using the field ?? 0.0 pattern, we can ensure that the compiler knows the field will never be null, even when asserts are not enabled. This optimization improves Flutter Gallery code size:

ARM64: -1.06%
ARM: -1.02%

/cc @mkustermann @alexmarkov @mraleph

@auto-assign auto-assign bot requested a review from GaryQian February 6, 2020 16:18
Comment on lines 16 to 19
: _dx = dx ?? 0.0
, _dy = dy ?? 0.0
, assert(dx != null)
, assert(dy != null);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can this be made consistent with the rest of the file? I think commas go at the end of the lines rather than the start.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it enough to just assert these are not null rather than coalescing? Here and below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather a user know they tried to pass null where they shouldn't than find out they passed null and got 0.

It's already an error to pass null based on the rest of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately we can't just add an assert, because the compiler needs to be able to prove that the field will never be null in release mode, where the asserts aren't triggered.

If the constructor wasn't const, we would throw an exception unconditionally instead. Is it not sufficient to prevent those kinds of errors with an assert in debug mode?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh that makes sense.

I think this is fine then. Thanks!

Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@dnfield
Copy link
Contributor

dnfield commented Feb 7, 2020

I bet applying this to the vector_math package would help too

@sjindel-google sjindel-google merged commit c932214 into master Feb 7, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 7, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 8, 2020
dnfield pushed a commit to flutter/flutter that referenced this pull request Feb 8, 2020
* c932214 Ensure fields of Rect and OffsetBase classes are optimized as non-null. (flutter/engine#16465)

* 5c70356 Simplify task queues locking mechanism (flutter/engine#16477)

* d589dde Fix text range logic for a11y (flutter/engine#16496)

* 1a4f4e3 Fix unused import in Android embedder (flutter/engine#16494)

* 964ae10 Disable ShellTest.WaitForFirstFrameTimeout on Fuchsia (flutter/engine#16495)

* 6158f03 Roll src/third_party/skia 87e3bef6f82f..9f3eef796f63 (7 commits) (flutter/engine#16493)
NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
…l. (flutter#16465)

* Ensure fields of Rect and OffsetBase classes are optimized as non-null.

* Update web_ui and formatting
NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
@tvolkert tvolkert deleted the sjindel.null branch March 31, 2020 03:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants