Kotlin: use Enum.entries instead of Enum.values()
#2182
Conversation
|
The CI seems to fail because it uses Kotlin 1.8.20 or lower 😅 |
jmartinesp
force-pushed
the
kotlin/use-entries-for-enums
branch
from
9c5d17b to
08c7643
Compare
…entries` usage on `Kotlin >= 1.9.0` and keep backwards compatibility with previous versions
jmartinesp
force-pushed
the
kotlin/use-entries-for-enums
branch
from
08c7643 to
bd83dbe
Compare
|
I added an |
| | `external_packages` | | A map of packages to be used for the specified external crates. The key is the Rust crate name, the value is the Kotlin package which will be used referring to types in that crate. See the [external types section of the manual](../udl/ext_types_external.md#kotlin) | ||
| | `android` | `false` | Used to toggle on Android specific optimizations | ||
| | `android_cleaner` | `android` | Use the [`android.system.SystemCleaner`](https://developer.android.com/reference/android/system/SystemCleaner) instead of [`java.lang.ref.Cleaner`](https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/ref/Cleaner.html). Fallback in both instances is the one shipped with JNA. | ||
| | `use_enum_entries` | `false` | Uses `EnumClass.entries` instead of `EnumClass.values()` in the bindings, which is more efficient as it reuses the same list instance every time. Note this is only available since Kotlin 1.9.0 |
There was a problem hiding this comment.
I like the idea of using a config value for this a lot. My one concern is that we'll end up with tons of these entries that people will need to fiddle with. What do you think of adding something like kotlin_target_version? The use_enum_entries method can check if it's >= 1.9.0. This same logic could be re-used for using other language features.
There was a problem hiding this comment.
A simple >= will only work if this is stored as a tuple though, not if it's stored as a string.
There was a problem hiding this comment.
That sounds like a sensible idea. Let me check if I can get something working real quick.
There was a problem hiding this comment.
This looks great, my one last concern is the use of cargo_metadata for the parsing. We're working towards making that optional, so I'd rather we did the parsing ourselves.
However, this is getting really far away from your original concerns though. Do you want to try to tackle this or should we merge it as-is and I can try to handle that one as a follow-up?
There was a problem hiding this comment.
I'd rather merge it as is, if possible. I didn't realise you were trying to get rid of cargo metadata. Maybe I should have used semver instead?
There was a problem hiding this comment.
Sounds good, let's merge this and figure out how to remove it later. I'm kind of thinking that we can just parse the versions by hand for now and avoid the extra dependency, but we can discuss in a separate PR.
There was a problem hiding this comment.
Oops, I see there's still a clippy error. I can merge once you fix that one.
There was a problem hiding this comment.
I hope 1dfd0a5 will fix it 🤞 .
jmartinesp
force-pushed
the
kotlin/use-entries-for-enums
branch
from
486fa87 to
75a06ea
Compare
Since Kotlin
1.9, the.entriesproperty is generated at build time for enum classes. UnlikeEnum.values(), this property doesn't instantiate a new array with every enum case every time it's used, instead a single List instance is reused.This should improve performance a tiny bit for Kotlin bindings. However, note this will only be supported since Kotlin 1.9, I don't know if there is a min version the project wants to support.