eed3si9n / sjson-new

a typeclass based JSON codec that's backend independent
Apache License 2.0
36 stars 19 forks source link

use ClassTag instead of ClassManifest #13

Closed xuwei-k closed 7 years ago

eed3si9n commented 8 years ago

Is this thread safe in Scala 2.10? https://issues.scala-lang.org/browse/SI-6240

xuwei-k commented 8 years ago

ummm, maybe thread safe :(

eed3si9n commented 8 years ago

More on this topic here - http://docs.scala-lang.org/overviews/reflection/thread-safety.html

dwijnand commented 7 years ago

ClassManifest is a (deprecated) type alias in Predef for scala.reflect.ClassManifest: https://github.com/scala/scala/blob/v2.10.6/src/library/scala/Predef.scala#L105

which is a (deprecated) type alias for not-deprecated scala.reflect.ClassTag: https://github.com/scala/scala/blob/v2.10.6/src/library/scala/reflect/package.scala#L28

So, although these types are quite magical in the compiler, I wouldn't be surprised if this change didn't make the code any less thread safe than it is already.


Also note that, confusingly, scala.reflect.Manifest was un-deprecated, while extending deprecated scala.reflect.ClassManifest: https://github.com/scala/scala/blob/v2.10.6/src/library/scala/reflect/Manifest.scala#L42-L44

(And, finally, for some more, confusing, trivia, all these type: scala.reflect.ClassManifest, scala.reflect.ClassTag, and scala.reflect.Manifest are actually in scala-library and not in scala-reflect, which kind of makes sense otherwise Predef wouldn't be able to define the type alias)

dwijnand commented 7 years ago

Further evidence that this change is good-to-go:

ClassTag is an exact drop-in replacement for ClassManifest

by @xeno-by in http://stackoverflow.com/questions/12218641/scala-what-is-a-typetag-and-how-do-i-use-it#comment16398232_12232195