-
Notifications
You must be signed in to change notification settings - Fork 49
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
Scala-native support #568
Scala-native support #568
Conversation
Nice! Is there a possibility of a snapshot, or should I bootleg this? Looking to unblock the fs2-data test suite. |
@armanbilge if you want it today you're gonna need to do a bootleg, but we'll set up snapshot publishing before the end of the week for the series/0.8 branch. |
private[weaver] object Timestamp { | ||
|
||
def format(epochSecond: Long): String = Zone { implicit zone => | ||
val out = alloc[time.tm]() |
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 this can be stackalloc
, no?
We're not return the pointer to the caller, so we don't need it on the heap, and the return value is a Scala String which will be on the heap anyways.
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.
And this makes Zone unnecessary as well
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.
yeah fair enough, I copied it verbatim from 0.4.0 code I had laying around
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.
Addressed aa7296e
|
||
def localTime(hours: Int, minutes: Int, seconds: Int): Long = Zone { | ||
implicit zone => | ||
val out = alloc[time.tm]() |
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.
same as above - the return is Long
, by value, so the memory can be allocated by stackalloc
- this would make it much, much faster as well
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.
Addressed aa7296e
continuation: Array[Task] => Unit): Unit = { | ||
val _ = executeFuture(eventHandler, loggers).map(_ => | ||
continuation(Array.empty[Task]))( | ||
scala.scalajs.concurrent.JSExecutionContext.queue) |
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.
Hmm, is this what we had before?
IIRC we are bringing the macrotask - do we want to use it instead? #476
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.
In this case it really shouldn't matter
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.
Hmm, is this what we had before?
Nah before we were using a callback based async method. We're unifying between JS and Native by relying on Future instead, and we're only needing an EC to plug the "end of the world" callback to the future.
@@ -28,8 +28,8 @@ abstract class DogFood[F[_]]( | |||
// for some time before getting the logs back. On JVM platform | |||
// we do not need to wait, since the suite will run synchronously | |||
private val patience: Option[FiniteDuration] = PlatformCompat.platform match { | |||
case JS => 2.seconds.some | |||
case JVM => none | |||
case JS | Native => 2.seconds.some |
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.
For my own understanding - the comment above talks about SJS being asynchronous, hence the need for waiting.
Do we still need it for Native, which will also execute synchronously? Or is it something else?
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.
Addressed aa7296e
I'm just gonna go ahead and merge this, we can PR the fs2 version later on against the 0.8 branch |
Adds Scala-Native support
Waiting for this to be merged and for the release train to start (we need CE + fs2 non-RC versions) : typelevel/cats-effect#3138