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

Minor, move decodeDuration logic to a function reference #2550

Merged
merged 1 commit into from
Jan 25, 2024

Conversation

udalov
Copy link
Member

@udalov udalov commented Jan 22, 2024

This is needed because once we enable indy lambdas by default (KT-45375), @SuppressAnimalSniffer annotation stops working, and the animalsnifferMain task reports several errors about java.time.Duration being used.

There are multiple ways to workaround this issue, for example we could annotate the lambda with @JvmSerializableLambda or compile the whole module with -Xlambdas=class, but I chose to use a simple function reference instead, since we don't generate those via invokedynamic yet (KT-45658), and it doesn't make the code any more difficult.

@udalov udalov requested a review from sandwwraith January 22, 2024 21:18
@sandwwraith
Copy link
Member

I take it that R8 supports indy lambdas? Ok then. Please add the same comment to the code.

Also, you've deleted the import and forgot to delete actual usage of annotation.

Copy link
Member

@sandwwraith sandwwraith left a comment

Choose a reason for hiding this comment

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

.

@udalov
Copy link
Member Author

udalov commented Jan 24, 2024

Maybe my commit message isn't clear. The annotation is still needed. If we compile this with without the annotation, animalsnifferMain will complain about java.time.Duration not available in older Android API. And extracting it to a function reference is required because animalsnifferMain does not handle indy lambdas correctly, which we want to enable in KT-45375.

R8 supports indy lambdas, but it is irrelevant here.

Import was optimized automatically because there's a *-import from the whole package already.

@sandwwraith
Copy link
Member

Import was optimized automatically because there's a *-import from the whole package already.

And yet, all builds fail with Compilation error: :kotlinx-serialization-hocon:compileKotlin: e: file:///mnt/agent/work/b2fef8360e1bcf3d/formats/hocon/src/main/kotlin/kotlinx/serialization/hocon/Hocon.kt:115:10 Unresolved reference: SuppressAnimalSniffer. This looks like a compiler bug, so explicit import may be a necessary workaround.

This is needed because once we enable indy lambdas by default
(KT-45375), `@SuppressAnimalSniffer` annotation stops working, and the
`animalsnifferMain` task reports several errors about
`java.time.Duration` being used.

There are multiple ways to workaround this issue, for example we could
annotate the lambda with `@JvmSerializableLambda` or compile the whole
module with `-Xlambdas=class`, but I chose to use a simple function
reference instead, since we don't generate those via invokedynamic yet
(KT-45658), and it doesn't make the code any more difficult.
@udalov udalov force-pushed the udalov/indy-lambdas branch from 50e5f4e to 6667909 Compare January 24, 2024 16:06
@udalov
Copy link
Member Author

udalov commented Jan 24, 2024

Yeah, sorry. 🤦 It was stupid of me not to check compilation one last time after optimizing imports.

@sandwwraith sandwwraith merged commit 41c0bb1 into dev Jan 25, 2024
@udalov udalov deleted the udalov/indy-lambdas branch January 25, 2024 11:01
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

2 participants