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.

Sign up for GitHub

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

Jump to bottom

Simplify PbList implementation, fix freezing #626

Merged
merged 27 commits into from
Jun 15, 2022
Merged

Simplify PbList implementation, fix freezing #626

merged 27 commits into from
Jun 15, 2022

Conversation

osa1
Copy link
Member

@osa1 osa1 commented Apr 23, 2022 •
edited
Loading

  • PbList is now a single class

  • Freezing no longer creates a new list, sets _isReadOnly flag of the current
    list

  • Freezing PbList now freezes elements

PbList implementation is now consistent with PbMap.

Fixes #624.

cl/450278877

@osa1 osa1 requested review from mraleph and sigurdm April 23, 2022 01:26
sigurdm
sigurdm approved these changes Apr 25, 2022
View reviewed changes
Copy link
Collaborator

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

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

Nice!

LGTM

protobuf/lib/src/protobuf/field_info.dart Outdated Show resolved Hide resolved
protobuf/lib/src/protobuf/field_info.dart Outdated Show resolved Hide resolved
protobuf/lib/src/protobuf/pb_list.dart Outdated Show resolved Hide resolved
protobuf/lib/src/protobuf/pb_list.dart Outdated Show resolved Hide resolved
@osa1 osa1 marked this pull request as ready for review April 28, 2022 12:32
@osa1
Copy link
Member Author

osa1 commented Apr 28, 2022

I refactored the implementation a little bit and I think this version looks good.

Only problem is removing FrozenPbList and PbListBase is a breaking change.

We could fix the two bugs (freezing the list allows updates in aliased lists, freezing the list does not freeze elements) without removing any classes, but it would require copying the mutable list (deep or shallow depending on the intended semantics) when creating a FrozenPbList. That will cause performance issues. The current PR should solve the issue with minimal runtime overhead.

I say we keep this PR for the next major release (maybe we can create a branch for it). Fixing the bug without changing the API requires more work in runtime.

@osa1 osa1 mentioned this pull request May 10, 2022
Merged
osa1 added a commit to osa1/protobuf.dart that referenced this pull request May 24, 2022
@osa1
Remove invalid check in _FieldSet.writeFieldValue
0138eaf 
A field value is one of these:

- PbListBase (for `repeated`, PbList after google#626)
- PbMap
- int
- double
- bool
- String
- List<int> (for `bytes`)
- GeneratedMessage

`ByteData` is not a valid field value type, so remove the `ByteData`
cases when checking field value types.
@osa1 osa1 mentioned this pull request May 24, 2022
Merged
osa1 added a commit that referenced this pull request May 25, 2022
@osa1
Remove invalid check in _FieldSet.writeFieldValue (#660)
c3b3af3 
A field value is one of these:

- PbListBase (for `repeated`, PbList after #626)
- PbMap
- int
- double
- bool
- String
- List<int> (for `bytes`)
- GeneratedMessage

`ByteData` is not a valid field value type, so remove the `ByteData`
cases when checking field value types.
@osa1 osa1 requested a review from sigurdm June 1, 2022 08:27
@osa1
Copy link
Member Author

osa1 commented Jun 1, 2022

GitHub's "request review" button doesn't seem work for non-collaborators.. @rakudrama PTAL.

sigurdm
sigurdm approved these changes Jun 2, 2022
View reviewed changes
Copy link
Collaborator

@sigurdm sigurdm left a comment

Choose a reason for hiding this comment

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

LGTM

@osa1 osa1 merged commit e61c178 into google:master Jun 15, 2022
@osa1 osa1 deleted the fix_pblist branch June 15, 2022 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rakudrama rakudrama rakudrama left review comments

@sigurdm sigurdm sigurdm approved these changes

@mraleph mraleph Awaiting requested review from mraleph

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PbList.toFrozenPbList does not freeze the list in-place, allows modifying frozen messages
3 participants
@osa1 @rakudrama @sigurdm