Introduce korlibs-concurrent (Lock + NativeThread) + add logger, datastructure and platform to K/N desktop targets #2132
Conversation
soywiz
changed the title
| private external val process: dynamic | ||
| private external val Deno: dynamic | ||
|
|
||
| private val isDenoJs: Boolean by lazy { js("(typeof Deno === 'object' && Deno.statSync)").unsafeCast<Boolean>() } |
There was a problem hiding this comment.
Why do you duplicate platform code in logger? Why is it a problem to depend on platform?
There was a problem hiding this comment.
Because just for two lines, I don't see adding an extra dependency in this case, like the JS ecosystem would do.
There was a problem hiding this comment.
Hopefully in Kotlin 2 we will have the ability to differentiate runtimes integrated in the stdlib at some point, so this will be removed ad the korlibs-platform will be reduced and eventually deleted.
|
|
||
| private fun getEnvs(): Map<String, String> { | ||
| val out = LinkedHashMap<String, String>() | ||
| val env = platform.posix.__environ |
There was a problem hiding this comment.
We should avoid so short variable names. It really makes the code hard to read.
There was a problem hiding this comment.
We could control env local variable, but not __environ since that comes from the operating system
|
|
||
| internal actual val miniEnvironmentVariables: Map<String, String> by lazy { getEnvs() } | ||
|
|
||
| private fun readStringsz(ptr: CPointer<WCHARVar>?): List<String> { |
There was a problem hiding this comment.
Is the z in the function name on purpose?
There was a problem hiding this comment.
Yes. The stringz is typically from C-languags world. It is a string that ends with a '\0' byte. Typically in C strings strings work like that instead of having the size attached, they just end by '\0'
| import korlibs.concurrent.lock.wait as waitConcurrent | ||
|
|
||
| @Deprecated("Use korlibs.concurrent.lock package") | ||
| typealias BaseLock = korlibs.concurrent.lock.BaseLock |
There was a problem hiding this comment.
Is this so you do not brake others? Could we use this to move the events to a new package?
There was a problem hiding this comment.
Yes for both questions. The thing is that we want to keep at least source-code compatibility for at least a few versions.
| //fun unlock() | ||
| } | ||
|
|
||
| //typealias Lock = BaseLock |
There was a problem hiding this comment.
We should not leave in comment code in PR. YAGNI: You aren't gonna need it. If you do, you can readd it from earlier commit.
There was a problem hiding this comment.
This is old code that was like that, I'm okay with removing those lines. So feel free to do so.
|
|
||
| package korlibs.concurrent.lock | ||
|
|
||
| import korlibs.concurrent.thread.* |
There was a problem hiding this comment.
- imports are not adviced since they make it hard to determine which classes are from which imports.
There was a problem hiding this comment.
Rationale behind the star imports:
KorGE uses star imports because it relies a lot on extension members. It was a pain in the ass adding all those members manually for people starting. Users typically don't care where classes come from as long as the code works.
There was a problem hiding this comment.
Yes, but it makes it pain in the ass to refactor. For our current task forexample it is important to see which methods are used from which packages, when we want to reduce the dependency towards a package.
There was a problem hiding this comment.
Why it is a pain in the ass? Doesn't @Deprecated has a parameter to specify replacement to do it automatically inside the IDE?
/CC @holloszaboakos