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

runtime polish #198

Merged
merged 37 commits into from
Oct 3, 2023
Merged

runtime polish #198

merged 37 commits into from
Oct 3, 2023

Conversation

andrewparmet
Copy link
Collaborator

@andrewparmet andrewparmet commented Sep 28, 2023

Changes:

  • Bytes now does not permit instantiation externally without making a defensive copy of the array. No-copy construction is Bytes.from(ktmessage).
  • Companion object methods are JvmStatic in common code and generated code
  • "runtime utils" functions are boxed and JvmStatic - Collections and SizeCodecs
  • Foo.FooDsl becomes Foo.Builder
  • Don't mess with removing explicit API keywords; instead, use two space indent to keep things somewhat clean and sane
  • Clean up ktlint settings re: trailing commas in auto formatting
  • Add JvmName annotations where public methods had strange Kotlin mangling for the JVM
  • Make UnknownFields fields JvmInline classes now that that API is stable (open to undoing this)

Comment on lines +61 to +63
private val _sizeOf: KFunction1<Int, Int> = SizeCodecs::sizeOf

val sizeOf = SizeCodecs::class.asTypeName().member(_sizeOf.name)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Get compile-safe references to these functions.

object Message : Nonscalar(ktRepresentation = KtMessage::class)
object String : Nonscalar(kotlin.String::class)
object Bytes : Nonscalar(protokt.v1.Bytes::class)
data object Enum : Nonscalar(ktRepresentation = KtEnum::class)
Copy link
Collaborator Author

@andrewparmet andrewparmet Sep 29, 2023

Choose a reason for hiding this comment

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

IDE recommended, shrug

Comment on lines -31 to -33
// strips Explicit API mode declarations
// https://kotlinlang.org/docs/whatsnew14.html#explicit-api-mode-for-library-authors
private fun stripApiMode(code: String) =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

meh

KtLintRuleEngine(
ruleProviders(),
editorConfigOverride = EditorConfigOverride.from(
INDENT_SIZE_PROPERTY to 2,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Leave KotlinPoet settings alone.

}

public final class protokt/v1/Bytes {
public static final field Companion Lprotokt/v1/Bytes$Companion;
public fun <init> ([B)V
public static final fun empty ()Lprotokt/v1/Bytes;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Improve Java interop.

Comment on lines +20 to +22
class Bytes internal constructor(
internal val value: ByteArray
) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Make defensive copies.

} else {
unmodifiableMap(map)
}
expect object Collections {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wrap these functions for polish and ability to use JvmStatic.

t + sizeOf(k, v).let { s ->
s + sizeOf(s)
@JvmStatic
fun <K, V> sizeOf(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

just call it sizeOf like the others

@@ -38,12 +44,14 @@ class Bytes(internal val value: ByteArray) {
companion object {
private val EMPTY = Bytes(ByteArray(0))

@JvmStatic
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could have sworn I tried to put JvmStatic in common code before and the compiler threw up - now that it's working, I'm going to consider adding it to the generated code in the right spots before going 1.0, if there are any right spots. Maybe the constructor functions if I put them in companion objects.


@JvmStatic
fun from(message: KtMessage) =
Bytes(message.serialize())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No-copy Bytes construction from a KtMessage.

@andrewparmet andrewparmet merged commit 58a6a7b into open-toast:main Oct 3, 2023
@andrewparmet andrewparmet deleted the runtime-polish branch October 3, 2023 15:02
ogolberg pushed a commit to ogolberg/protokt that referenced this pull request Jan 2, 2024
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