-
Notifications
You must be signed in to change notification settings - Fork 80
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
Remove fastutil dependency #155
Conversation
Interesting.
Moreover, it isn't compatible with K2 if use KMP, see https://youtrack.jetbrains.com/issue/KT-66723 (but I've written a workaround). |
|
||
actual class BitSet actual constructor(size: Int): java.util.BitSet(size){ | ||
actual val size = size() | ||
} | ||
|
||
actual typealias IntStack = IntArrayList | ||
actual class IntStack: Stack<Int>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if you replace IntArrayList
with Stack<Int>
, actual/expect
classes become redundant and they can be replaced just with Stack<Int>
. It's declared in a common source set and it's currently used for js
and native
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, i chose to leave it since IntStack
is used in generated code, I wanted to keep the PR smaller. But I can make that change if one of the maintainers wants me to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
Looking forward to this 🤟 (trying to use Clikt in Lambda function) |
All hail the good folk in the Markdown team. Please consider merging this lovely request & doing a new release 😅 |
As shown in ajalt/clikt#507, the fastutil dependency is over 6MB in size. This dependency is only used in a single line.
This PR removes that dependency and uses the same
Stack<Int>()
on JVM as on the other platforms.To make sure this doesn't affect performance, I used the following JMH benchmark that converts the
gitBook.md
file from the test data to html:Performance on
master
, with fastutilIntStack
:Performance after this PR, with
Stack<Int>()
:According to this benchmark, the change has no performance impact.