Skip to content

Conversation

KosmX
Copy link

@KosmX KosmX commented Sep 12, 2025

ProtoBuf's ProtoPacked annotation only worked for the original signed primitives and ByteArray, and did not work for unsigned variants.

…s can be packed.

inline popTag -> popTagOrDefault is needed, because packed serializer will use MISSING_TAG for entries, popTag fails on that.
@KosmX KosmX marked this pull request as ready for review September 12, 2025 23:36
@fzhinkin
Copy link
Contributor

@KosmX, thanks for opening the PR!

Related issue: #3045

@fzhinkin fzhinkin self-assigned this Sep 15, 2025
@fzhinkin
Copy link
Contributor

Before moving further with this PR (for the part where inline value classes support is fixed) we need to address the questions from #3045 (comment)

@KosmX
Copy link
Author

KosmX commented Sep 15, 2025

Okay, I'm not a Kotlin maintainer or anything, but here are my thoughts:
I'll start with the last, that's probably the easiest or most important

Kotlin might start supporting multi-field value classes at some point, it's worth considering them when addressing single-field value classes issue

As long as value classes use their default serializer, I think, they should be inlined completely:
For example I have a value class Complex(val real: Double, val imaginary: Double), I want it to be extremely compact in a huge array.
And that's the same for serialization, especially for binary formats like protobuf.

And, if for some reason, I do not want to serialize the value class this way, I can still go the long way, and write a custom serializer, or maybe we can add an annotation for serializable value classes (like DontInlineSerialize)

If we want to match standards/specifications exactly, don't bend protobuf's rules, maybe do the inlining the opposite way: inline only for unsigned primitives, and for explicit SerializeInline annotation.

Currently we try to inline things in all formats.

potentially, there might be the same issues for inline values classes that are fields of regular classes, we have to figure out something there too

The way I edited the isPackable getter, it is recursive on nested value classes. I should write a test-case for that.

we need to figure out how to deal with Proto-annotated fields of inline value classes

If the value class is inlined, I would look for these annotations recursively, from the deepest.

On the implementation question, I have some ideas, but nothing precise-enough, and I think it would be better to have the complete specification first, maybe involve more people, ask what their thoughts are.

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.

2 participants