KE2: extract binary operators #17761
Conversation
tamasvajk
force-pushed
the
ke2-binary-ops
branch
from
4f4dcd3
to
a19b5e6
Compare
tamasvajk
force-pushed
the
ke2-binary-ops
branch
from
118183e
to
dfdf566
Compare
tamasvajk
force-pushed
the
ke2-binary-ops
branch
2 times, most recently
from
9d443b1
to
1e0e951
Compare
tamasvajk
force-pushed
the
ke2-binary-ops
branch
from
1e0e951
to
a90f76e
Compare
tamasvajk
force-pushed
the
ke2-binary-ops
branch
from
a90f76e
to
db13b32
Compare
| @@ -297,37 +298,83 @@ private fun KotlinFileExtractor.extractBinaryExpression( | |||
| val op = expression.operationToken | |||
| val target = expression.resolveCallTarget()?.symbol | |||
There was a problem hiding this comment.
What's the plan for target == null?
If we're just going to emit an errorExpr, and perhaps add some more info in a supplementary table, then can we handle that case first, and make isNumericWithName and isBinaryPlus take a non-null KaFunctionSymbol and not have to handle the null case?
There was a problem hiding this comment.
There are cases below where target is null, for example in case of ===.
| if (op == KtTokens.PLUS && target.isBinaryPlus()) { | ||
| extractBinaryExpression(expression, callable, parent, tw::writeExprs_addexpr) | ||
| } else if (op == KtTokens.MINUS && target.isNumericWithName("minus")) { | ||
| extractBinaryExpression(expression, callable, parent, tw::writeExprs_subexpr) |
There was a problem hiding this comment.
So far at least this is very regular; I wonder if it would be worth writing as
val writeExprs_fun = if (op == KtTokens.PLUS && target.isBinaryPlus()) { tw::writeExprs_addexpr }
else if ...
else null
if (writeExprs_fun != null) {
extractBinaryExpression(expression, callable, parent, writeExprs_fun
} else {
something_else
}
?
And either way, something like
when {
op == KtTokens.PLUS && target.isBinaryPlus() -> tw::writeExprs_addexpr // or -> extractBinaryExpression(expression, callable, parent, tw::writeExprs_addexpr)
...
else -> null
}
might be nicer than a big if/elseif
There was a problem hiding this comment.
Here's the followup refactoring PR: #17974
| KtTokens.GT -> extractBinaryExpression(expression, callable, parent, tw::writeExprs_gtexpr) | ||
| KtTokens.LTEQ -> extractBinaryExpression(expression, callable, parent, tw::writeExprs_leexpr) | ||
| KtTokens.GTEQ -> extractBinaryExpression(expression, callable, parent, tw::writeExprs_geexpr) | ||
| else -> TODO("error") |
There was a problem hiding this comment.
If we're changing this to just return tw::writeExprs_ltexpr as above, then we could drop the op in listOf check and just return null in the else case.
| } | ||
| } else { | ||
| TODO("Extract lowered equivalent call, such as `a.compareTo(b) < 0`") |
There was a problem hiding this comment.
Ah, but my previous suggestion won't work if we need to do something special in this case.
I think we should add a comment explaining what's going on with the AST in that case.
There was a problem hiding this comment.
I don't think we need the comment. Instead we should merge this too: #17939
| this.hasMatchingNames( | ||
| CallableId(FqName("kotlin"), null, Name.identifier("plus")), | ||
| ClassId(FqName("kotlin"), Name.identifier("String")), | ||
| nullability = KaTypeNullability.NULLABLE, |
There was a problem hiding this comment.
I think it would be worth adding a comment saying what this last case corresponds to
There was a problem hiding this comment.
I can dig that up, but this bit was just refactored. It was originally introduced in #17752
There was a problem hiding this comment.
Comment added in 53460d7.
No description provided.