Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update getList and getMap return types #903

Merged
merged 6 commits into from
Nov 23, 2023

Conversation

osa1
Copy link
Member

@osa1 osa1 commented Nov 22, 2023

This improves iteration performance by making moveNext and current direct calls in kernel. #902 does not have the same effect on moveNext and current calls, this change is needed even with #902.

This change was not possible before #880 as users could override the list and map types to types that are not PbLists or PbMaps.

Note: changes in the message field types (the plugin changes in this PR) are not necessary for the kernel improvements, but it doesn't hurt to generate more precise types everywhere.


cl/584816240

This improves iteration performance by making `moveNext` and `current`
direct calls in kernel. google#902 does not have the same effect on `moveNext`
and `current` calls, this change is needed even with google#902.

This change was not possible before google#880 as users could override the
list and map types to types that are not `PbList`s or `PbMap`s.
@osa1 osa1 requested a review from mkustermann November 22, 2023 10:30
Copy link
Collaborator

@mkustermann mkustermann left a comment

Choose a reason for hiding this comment

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

Nice!

protobuf/lib/src/protobuf/field_set.dart Outdated Show resolved Hide resolved
@osa1 osa1 merged commit cf43230 into google:master Nov 23, 2023
17 of 18 checks passed
@osa1 osa1 deleted the osa1/getMap_list_return_type branch November 23, 2023 08:29
@vvanm
Copy link

vvanm commented Feb 27, 2024

Hi,

When will this change hit the pub.dev protobuf package?

Latest version on pub.dev is from Published 6 months ago ago

Vincent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants