-
Notifications
You must be signed in to change notification settings - Fork 63
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
kotlin: Improve access of companions object fields/methods in Java #201
Conversation
@@ -1989,7 +1994,7 @@ internal fun peakMoonShadow(searchCenterTime: Time): ShadowInfo { | |||
val window = 0.03 // days before/after new moon to search for minimum shadow distance | |||
val t1 = searchCenterTime.addDays(-window) | |||
val t2 = searchCenterTime.addDays(+window) | |||
val tx = search(moonShadowSlopeContext, t1, t2, 1.0) ?: | |||
val tx = search(t1, t2, 1.0, moonShadowSlopeContext) ?: |
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.
This should've been done in the previous PR, not sure how I've missed it...
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.
OK, I just pushed this fix already. I figured we were both changing things at the same time?
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.
True, and great that git didn't need a rebase! :)
@@ -473,13 +473,15 @@ class Time private constructor( | |||
internal fun julianMillennia() = tt / DAYS_PER_MILLENNIUM | |||
|
|||
companion object { | |||
@JvmStatic | |||
private val origin = GregorianCalendar(TimeZoneUtc).also { |
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.
probably only public fields and methods should be marked with static but well maybe it instructs the compiler to optimize companion object altogether or not, not sure but sure it should be harmless.
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 seem to remember we talked about whether we want to get rid of using GregorianCalendar
, because it brings in a dependency on Java. Is that correct? I think you said if someone wants to use the Kotlin version of Astronomy Engine to create WebAssembly, this could be a problem. I think there a few other minor uses of Java symbols. It is possible to do something like the C version, where we make our own date/time functions from scratch.
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.
Oh sure, you can just take unix timestamp, the core data java.util.Date holds, https://docs.oracle.com/javase/7/docs/api/java/util/Date.html#getTime()
This proposed change is roughly separate matter from what this change is about, https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.jvm/-jvm-inline/ is considered to be common between Kotlin targets mainly for Java support for sure.
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.
OK, I think you meant @JvmStatic
, not @JvmInline
, but it looks like @JvmStatic
is also safe to use outside the JVM (if I understand the documentation correctly). I just keep thinking about this, and yes, I understand it is a separate issue. I just wanted to understand it. Thank you!
Background: For some reason Kotlin doesn't have static fields and methods in language level like the way exists in Java / C# and instead got companion objects to act like them.
In order to make those companion objects more accessible in Java level, there is
@JvmStatic
annotation which marking all the things with it which is a common pattern,See the following issue which we actually have also about useless warnings Dokka gives about companion object documentation, Kotlin/dokka#200
Aside from the problem which we also have, this is actually just doing such thing here for better accessibility in Java.