KE2: Extract binary plus on numeric types #17729
Conversation
| return | ||
| } | ||
|
|
||
| TODO() |
There was a problem hiding this comment.
I think ideally we would minimise the number of mid-method returns, so
} else {
TODO()
}
(without the return) would be better.
There was a problem hiding this comment.
I have no clear preference. I think this will become a three-long if-else chain: we'd need to handle string.plus and the method call variant for the non-special cases.
|
|
||
| extractExprContext(id, tw.getLocation(expression), callable, exprParent.enclosingStmt) | ||
| extractExpressionExpr(expression.left!!, callable, id, 0, exprParent.enclosingStmt) | ||
| extractExpressionExpr(expression.right!!, callable, id, 1, exprParent.enclosingStmt) |
There was a problem hiding this comment.
Aren't we going to want to do these 3 lines in every case, after the when (op) has finished?
There was a problem hiding this comment.
I don't know, maybe. In many cases X + Y will be extracted as X.opPlus(Y), so as a method call. Would we want to handle this case together with the above? Maybe we could, because the child indices are the same.
There was a problem hiding this comment.
Ah, good point. Maybe best to leave it as-is for now and possibly refactor later.
| } | ||
| */ | ||
|
|
||
| private fun isFunction( |
There was a problem hiding this comment.
I wonder if something like isFunctionNamed would be clearer. And maybe add a comment
/**
* True iff `symbol` is the function `packageName.className::functionName`
*/
(I'm not sure if I have the best punctuation in packageName.className::functionName OTTOMH)
There was a problem hiding this comment.
The KE1 equivalent of this function has some extra parameters, such as nullability, so adding Named would not necessarily make sense.
There was a problem hiding this comment.
OK; I don't like the name isFunction, because it sounds like it tells you whether something is a function or something else (e.g. a class), but perhaps it's too early to pick the best name.
| } | ||
|
|
||
| context(KaSession) | ||
| private fun KotlinFileExtractor.extractBinaryExpression( |
There was a problem hiding this comment.
I think a doc comment here would be useful, saying that sometimes these map to primitives like DbAddExpr, whereas at other times they are overloads (and give an example of how an overload of plus is defined in Kotlin).
No description provided.