JetBrains-Research / viktor

Efficient f64-only ndarray in Kotlin
MIT License
125 stars 6 forks source link

Avoid using vararg in API #30

Open altavir opened 4 years ago

altavir commented 4 years ago

Both because it complicates function calls, especially when vararg is not the last argument. Also there is possible performance impact because of array re-wrapping in kotlin.

dievsky commented 4 years ago

As far as I can tell, the only API methods that use vararg are also the ones where its use is unavoidable, like the general getter/setter methods. Which methods were meant exactly?

altavir commented 4 years ago

It is better to use IntArray for that. You can then add vararg extension on top of that. The problem with vararg is that when you need to pass thtough some arguments to setter from some other function, you need to use spread operator * and Kotlin usually allocates additional array on each spread call (it was the problem some time ago, but I am not sure if it was fixed). Also if you have signature like operator fun set(vararg indices: Int, value: Double), the only way to properly call it as a function is like this: set(value = 1.0, indices = *intArrayOf(2,3)), which is really inconvenient.

dievsky commented 4 years ago

So you're saying that we should provide both IntArray and vararg signature? This does sound reasonable.

the only way to properly call it as a function

The vararg setter can be called in two ways:

  1. as an operator: a[1, 1, 1, 1] = 42.0, or
  2. as a function: a.set(1, 1, 1, 1, value = 42.0)

No manual spreading and array-creation is required in either of them. This probably should be explicitly mentioned in the KDoc, though.

dievsky commented 4 years ago

Turns out we can't provide both IntArray and vararg signature, since they cause a JVM clash.

altavir commented 4 years ago

It is easily solved by @JvmName annotation. Even better - you can move vararg one to extensions. Here is the example.

altavir commented 4 years ago

About setter, you are correct. My problem was with this construct: f64Buffer.set(value = value, indices = *index).

dievsky commented 4 years ago

I wonder why does the extension approach work, while defining the method inside the class does not.

dievsky commented 4 years ago

f64Buffer.set(*index, value = value) looks a little better, though it doesn't solve the performance impact of wrapping and spreading.

altavir commented 4 years ago

Because extension is a static method in YourClassKt from JVM perspective. It is not in the same class.

dievsky commented 3 years ago

Also, F64Array.full is not fun to use at all.

altavir commented 3 years ago

Not fun, definitely :)