Skip to content

Simplify PbList implementation, fix freezing - #626

Merged
osa1 merged 27 commits into
google:masterfrom
osa1:fix_pblist
Jun 15, 2022
Merged

Simplify PbList implementation, fix freezing#626
osa1 merged 27 commits into
google:masterfrom
osa1:fix_pblist

Conversation

@osa1

@osa1 osa1 commented Apr 23, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

  • 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
osa1
osa1 commented Apr 23, 2022
View reviewed changes
Comment thread protobuf/lib/src/protobuf/pb_list.dart Resolved
sigurdm
sigurdm approved these changes Apr 25, 2022
View reviewed changes

@sigurdm sigurdm left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice!

LGTM

Comment thread protobuf/lib/src/protobuf/field_info.dart Resolved
Comment thread protobuf/lib/src/protobuf/field_info.dart Resolved
Comment thread protobuf/lib/src/protobuf/pb_list.dart Resolved
Comment thread protobuf/lib/src/protobuf/pb_list.dart Resolved
@osa1
osa1 marked this pull request as ready for review April 28, 2022 12:32
osa1
osa1 commented Apr 28, 2022
View reviewed changes
Comment thread protobuf/lib/src/protobuf/pb_list.dart Resolved
@osa1

osa1 commented Apr 28, 2022

Copy link
Copy Markdown
Contributor Author

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
sigurdm
sigurdm reviewed May 23, 2022
View reviewed changes
Comment thread protobuf/lib/src/protobuf/field_info.dart Resolved
@osa1 osa1 mentioned this pull request May 24, 2022
Merged
rakudrama
rakudrama reviewed May 24, 2022
View reviewed changes
Comment thread protobuf/lib/src/protobuf/pb_list.dart Resolved
rakudrama
rakudrama reviewed May 24, 2022
View reviewed changes
Comment thread protobuf/lib/src/protobuf/pb_list.dart Resolved
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

osa1 commented Jun 1, 2022

Copy link
Copy Markdown
Contributor Author

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

@sigurdm sigurdm left a comment

Copy link
Copy Markdown
Collaborator

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

@sigurdm sigurdm sigurdm approved these changes
@mraleph mraleph Awaiting requested review from mraleph
+1 more reviewer
@rakudrama rakudrama rakudrama left review comments
Reviewers whose approvals may not affect merge requirements

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