You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "mdedetrich (via GitHub)" <gi...@apache.org> on 2023/05/09 07:44:22 UTC
[GitHub] [incubator-pekko] mdedetrich opened a new pull request, #316: Add OptionConverters.toScala methods for java Optional primitives
mdedetrich opened a new pull request, #316:
URL: https://github.com/apache/incubator-pekko/pull/316
As pointed out in https://github.com/apache/incubator-pekko-http/pull/142#pullrequestreview-1417421049 we should do our best to avoid usage of the `Rich*` implicit classes within Java code especially since Java doesn't do inlining (unlike Scala).
I checked the pekko-http code and within Java sources we only use the `.toScala` methods (the Scala to Java conversion cases are done within Scala source which uses the standard extension methods).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on pull request #316: Add OptionConverters.toScala methods for java Optional primitives
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #316:
URL: https://github.com/apache/incubator-pekko/pull/316#issuecomment-1539993569
@pjfanning @He-Pin @nvollmar Would you be able to review this? Its the last PR needed for https://github.com/apache/incubator-pekko-http/pull/142
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich merged pull request #316: Add OptionConverters.toScala methods for java Optional primitives
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich merged PR #316:
URL: https://github.com/apache/incubator-pekko/pull/316
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #316: Add OptionConverters.toScala methods for java Optional primitives
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #316:
URL: https://github.com/apache/incubator-pekko/pull/316#discussion_r1188260033
##########
actor/src/main/scala-2.12/org/apache/pekko/util/OptionConverters.scala:
##########
@@ -21,9 +21,21 @@ import java.util._
@InternalStableApi
private[pekko] object OptionConverters {
import scala.compat.java8.OptionConverters.SpecializerOfOptions
+ import scala.compat.java8.OptionConverters._
@inline final def toScala[A](o: Optional[A]): Option[A] = scala.compat.java8.OptionConverters.toScala(o)
+ // The rest of the .toScala methods that work with OptionalDouble/OptionalInt/OptionalLong have to be manually
+ // redefined because the scala.compat.java8.OptionConverters.toScala variants work with scala.lang primitive types
+ // where as scala.jdk.javaapi.OptionConverters.toScala works with java.lang primitive types. Since the primary
+ // usecase of these functions is for calling within Java code its preferrable to return Java primitives, see
+ // https://github.com/scala/bug/issues/4214
+ def toScala(o: OptionalDouble): Option[java.lang.Double] = if (o.isPresent) Some(o.getAsDouble) else None
Review Comment:
I am intentionally not using `@inline` here because its the actual definition of the method being placed here, not another method call. JVM hotspot should inline these definitions without issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #316: Add OptionConverters.toScala methods for java Optional primitives
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #316:
URL: https://github.com/apache/incubator-pekko/pull/316#discussion_r1188516014
##########
actor/src/main/scala-2.12/org/apache/pekko/util/OptionConverters.scala:
##########
@@ -21,9 +21,21 @@ import java.util._
@InternalStableApi
private[pekko] object OptionConverters {
import scala.compat.java8.OptionConverters.SpecializerOfOptions
+ import scala.compat.java8.OptionConverters._
@inline final def toScala[A](o: Optional[A]): Option[A] = scala.compat.java8.OptionConverters.toScala(o)
+ // The rest of the .toScala methods that work with OptionalDouble/OptionalInt/OptionalLong have to be manually
+ // redefined because the scala.compat.java8.OptionConverters.toScala variants work with scala.lang primitive types
+ // where as scala.jdk.javaapi.OptionConverters.toScala works with java.lang primitive types. Since the primary
+ // usecase of these functions is for calling within Java code its preferrable to return Java primitives, see
+ // https://github.com/scala/bug/issues/4214
+ def toScala(o: OptionalDouble): Option[java.lang.Double] = if (o.isPresent) Some(o.getAsDouble) else None
Review Comment:
There is an idea to enable the inliner for Scala 2.12/Scala 2.13 which will be relevant later on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] mdedetrich commented on a diff in pull request #316: Add OptionConverters.toScala methods for java Optional primitives
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #316:
URL: https://github.com/apache/incubator-pekko/pull/316#discussion_r1188260033
##########
actor/src/main/scala-2.12/org/apache/pekko/util/OptionConverters.scala:
##########
@@ -21,9 +21,21 @@ import java.util._
@InternalStableApi
private[pekko] object OptionConverters {
import scala.compat.java8.OptionConverters.SpecializerOfOptions
+ import scala.compat.java8.OptionConverters._
@inline final def toScala[A](o: Optional[A]): Option[A] = scala.compat.java8.OptionConverters.toScala(o)
+ // The rest of the .toScala methods that work with OptionalDouble/OptionalInt/OptionalLong have to be manually
+ // redefined because the scala.compat.java8.OptionConverters.toScala variants work with scala.lang primitive types
+ // where as scala.jdk.javaapi.OptionConverters.toScala works with java.lang primitive types. Since the primary
+ // usecase of these functions is for calling within Java code its preferrable to return Java primitives, see
+ // https://github.com/scala/bug/issues/4214
+ def toScala(o: OptionalDouble): Option[java.lang.Double] = if (o.isPresent) Some(o.getAsDouble) else None
Review Comment:
I am intentionally not using `@inline` here because its the actual definition of the method being placed here, not another method call. JVM will inline these statements without issues.
##########
actor/src/main/scala-2.12/org/apache/pekko/util/OptionConverters.scala:
##########
@@ -21,9 +21,21 @@ import java.util._
@InternalStableApi
private[pekko] object OptionConverters {
import scala.compat.java8.OptionConverters.SpecializerOfOptions
+ import scala.compat.java8.OptionConverters._
@inline final def toScala[A](o: Optional[A]): Option[A] = scala.compat.java8.OptionConverters.toScala(o)
+ // The rest of the .toScala methods that work with OptionalDouble/OptionalInt/OptionalLong have to be manually
+ // redefined because the scala.compat.java8.OptionConverters.toScala variants work with scala.lang primitive types
+ // where as scala.jdk.javaapi.OptionConverters.toScala works with java.lang primitive types. Since the primary
+ // usecase of these functions is for calling within Java code its preferrable to return Java primitives, see
+ // https://github.com/scala/bug/issues/4214
+ def toScala(o: OptionalDouble): Option[java.lang.Double] = if (o.isPresent) Some(o.getAsDouble) else None
Review Comment:
I am intentionally not using `@inline` here because its the actual definition of the method being placed here, not another method call. JVM should inline these statements without issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #316: Add OptionConverters.toScala methods for java Optional primitives
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #316:
URL: https://github.com/apache/incubator-pekko/pull/316#discussion_r1188308742
##########
actor/src/main/scala-2.12/org/apache/pekko/util/OptionConverters.scala:
##########
@@ -21,9 +21,21 @@ import java.util._
@InternalStableApi
private[pekko] object OptionConverters {
import scala.compat.java8.OptionConverters.SpecializerOfOptions
+ import scala.compat.java8.OptionConverters._
@inline final def toScala[A](o: Optional[A]): Option[A] = scala.compat.java8.OptionConverters.toScala(o)
+ // The rest of the .toScala methods that work with OptionalDouble/OptionalInt/OptionalLong have to be manually
+ // redefined because the scala.compat.java8.OptionConverters.toScala variants work with scala.lang primitive types
+ // where as scala.jdk.javaapi.OptionConverters.toScala works with java.lang primitive types. Since the primary
+ // usecase of these functions is for calling within Java code its preferrable to return Java primitives, see
+ // https://github.com/scala/bug/issues/4214
+ def toScala(o: OptionalDouble): Option[java.lang.Double] = if (o.isPresent) Some(o.getAsDouble) else None
Review Comment:
I think that's fine, @inline is only applied if you compile with it
And in kotlin and scala3 we have `inline` keyword.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org
[GitHub] [incubator-pekko] He-Pin commented on a diff in pull request #316: Add OptionConverters.toScala methods for java Optional primitives
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #316:
URL: https://github.com/apache/incubator-pekko/pull/316#discussion_r1188308742
##########
actor/src/main/scala-2.12/org/apache/pekko/util/OptionConverters.scala:
##########
@@ -21,9 +21,21 @@ import java.util._
@InternalStableApi
private[pekko] object OptionConverters {
import scala.compat.java8.OptionConverters.SpecializerOfOptions
+ import scala.compat.java8.OptionConverters._
@inline final def toScala[A](o: Optional[A]): Option[A] = scala.compat.java8.OptionConverters.toScala(o)
+ // The rest of the .toScala methods that work with OptionalDouble/OptionalInt/OptionalLong have to be manually
+ // redefined because the scala.compat.java8.OptionConverters.toScala variants work with scala.lang primitive types
+ // where as scala.jdk.javaapi.OptionConverters.toScala works with java.lang primitive types. Since the primary
+ // usecase of these functions is for calling within Java code its preferrable to return Java primitives, see
+ // https://github.com/scala/bug/issues/4214
+ def toScala(o: OptionalDouble): Option[java.lang.Double] = if (o.isPresent) Some(o.getAsDouble) else None
Review Comment:
I think that's fine, @inline is only applied if you compile with it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org