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

Add an encodeCollection extension #1749

Merged
merged 6 commits into from Nov 22, 2021
Merged

Conversation

ansman
Copy link
Contributor

@ansman ansman commented Nov 1, 2021

This is akin to encodeStructure but for collections.

This fixes #1748

@qwwdfsad qwwdfsad self-requested a review November 1, 2021 14:41
Comment on lines 493 to 513
public inline fun Encoder.encodeCollection(
descriptor: SerialDescriptor,
collectionSize: Int,
block: CompositeEncoder.() -> Unit
) {
val composite = beginCollection(descriptor, collectionSize)
var ex: Throwable? = null
try {
composite.block()
} catch (e: Throwable) {
ex = e
throw e
} finally {
// End structure only if there is no exception, otherwise it can be swallowed
if (ex == null) composite.endStructure(descriptor)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the way that collections work, it would make sense to put the element iteration in the helper, not just the structure part. Why not use something like:

fun encodeCollection(
    descriptor: SerialDescriptor,
    collection: Collection<T>,
    encodeElement: CompositeEncoder.(T) -> Unit
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can encode things using a collection structure that aren't collections themselves. The simplest example are arrays.

I don't understand the first part, are you talking about which file the extension lives in or something else?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ansman The first part refers to the idea that you would possibly want encodeCollection to actually enforce/provide the expected semantics of a collection rather than leaving this to the caller. As to arrays, you can always overload the helper for array types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like that idea but I don't want to reduce the flexibility of not enforcing a collection. Instead two extensions could be added (one that accepts a collection size and one that accepts a collection).

Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of it!

Could you please also make the new signatures right?

It's our mistake that we haven't marked the original signatures as crossinline, so the ugly workaround with catch is necessary. We aim to break it (on the source level) in 1.4.0, but these can be done in the desired way from the very beginning.

With crossinline, it can be as straightforward as start() block() end() without catch and finally blocks

@ansman
Copy link
Contributor Author

ansman commented Nov 16, 2021

Sure can!

Copy link
Member

@qwwdfsad qwwdfsad left a comment

Choose a reason for hiding this comment

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

Thanks!

@qwwdfsad qwwdfsad merged commit 53b46e9 into Kotlin:dev Nov 22, 2021
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.

None yet

3 participants