Skip to content

Conversation

@chunhtai
Copy link
Contributor

@chunhtai chunhtai commented Feb 4, 2025

Reduce magic number

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added platform-android Android applications specifically engine flutter/engine related. See also e: labels. labels Feb 4, 2025
@chunhtai chunhtai requested a review from gmackall February 4, 2025 21:37
update.insert(std::make_pair(0, node0));

std::vector<uint8_t> expected_buffer(196);
std::vector<uint8_t> expected_buffer(
Copy link
Member

@gmackall gmackall Feb 4, 2025

Choose a reason for hiding this comment

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

Does this and the one above not need to be 4 * kBytesPerNode? Is that not what it was equal to before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is only 1 node in the update in this test, so technically it will be 1 * kBytesPerNode. If your question is about int32_t is 4 byte, that 4 is already take into account when calculating kBytesPerNode

Copy link
Member

@gmackall gmackall left a comment

Choose a reason for hiding this comment

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

One question, but approving regardless because I assume if it does need to be 4 times the value, the test will fail anyways! Thanks for following up

@chunhtai
Copy link
Contributor Author

chunhtai commented Feb 4, 2025

the byte per node is already taking the 4 into account.

@chunhtai chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 4, 2025
@gmackall
Copy link
Member

gmackall commented Feb 4, 2025

Whoops, yeah I think I assumed the wrong number of nodes in the first pr, and carried that assumption over to this PR. LGTM!

@auto-submit
Copy link
Contributor

auto-submit bot commented Feb 4, 2025

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

@auto-submit auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Feb 4, 2025
@chunhtai chunhtai added the autosubmit Merge PR when tree becomes green via auto submit App label Feb 4, 2025
@auto-submit auto-submit bot added this pull request to the merge queue Feb 5, 2025
Merged via the queue into flutter:master with commit b0d9546 Feb 5, 2025
178 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Feb 5, 2025
<!--
Thanks for filing a pull request!
Reviewers are typically assigned within a week of filing a request.
To learn more about code review, see our documentation on Tree Hygiene:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
-->

Reduce magic number

## Pre-launch Checklist

- [ ] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [ ] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [ ] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [ ] I signed the [CLA].
- [ ] I listed at least one issue that this PR fixes in the description
above.
- [ ] I updated/added relevant documentation (doc comments with `///`).
- [ ] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [ ] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [ ] All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Discord]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
@flutter-dashboard flutter-dashboard bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Feb 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 5, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 6, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 7, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 8, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 8, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 9, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 9, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 10, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 10, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 10, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 10, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 10, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 11, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 11, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 11, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 11, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Feb 11, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 20, 2025
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

engine flutter/engine related. See also e: labels. platform-android Android applications specifically

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants