-
Notifications
You must be signed in to change notification settings - Fork 1
TASK Binary expression ast with only two operands: left and right #16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…perands: `true && true && true`
…of multiple operands i have the feeling this will make working with the BinaryOperandNode (eg type resolving/inference) easier. Also the php and js AST use the $left $right and $operator as a sideeffect it transpiles - transpile `true === true === true` correctly (with Parenthesis): `(true === true) === true` without Parenthesis its invalid php
| public function render(): string | ||
| { | ||
| return (string) (0 + 1234567890 + 42 + 0b10000000000000000000000000000000 + 0b01111111100000000000000000000000 + 0b00000000011111111111111111111111 + 0o755 + 0o644 + 0xFFFFFFFFFFFFFFFFF + 0x123456789ABCDEF + 0xA + 1E3 + 2e6 + 123.456 + 0.1e2 + .22); | ||
| return (string) (((((((((((((((0 + 1234567890) + 42) + 0b10000000000000000000000000000000) + 0b01111111100000000000000000000000) + 0b00000000011111111111111111111111) + 0o755) + 0o644) + 0xFFFFFFFFFFFFFFFFF) + 0x123456789ABCDEF) + 0xA) + 1E3) + 2e6) + 123.456) + 0.1e2) + .22); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you prefer not having those parenthesis here, i experimented with a shouldAddParenthesisIfNecessary flag, this way we could transpile without parenthesis (if the operand is the same in chained binary ops)
public function transpile(BinaryOperationNode $binaryOperationNode): string
{
$expressionTranspiler = new ExpressionTranspiler(
scope: $this->scope,
shouldAddQuotesIfNecessary: true,
shouldAddParenthesisIfNecessary: !(
$binaryOperationNode->operands->first->root instanceof BinaryOperationNode
&& $binaryOperationNode->operands->first->root->operator === $binaryOperationNode->operator
)
);
$first = $expressionTranspiler->transpile($binaryOperationNode->operands->first);
$operator = $this->transpileBinaryOperator($binaryOperationNode->operator);
$second = $expressionTranspiler->transpile($binaryOperationNode->operands->second);
if ($this->shouldAddParenthesis) {
return sprintf('(%s %s %s)', $first, $operator, $second);
} else {
return sprintf('%s %s %s', $first, $operator, $second);
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't mind those :) All's good 👍
grebaldi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Tests are green
- One fewer class
- Waaaay better, semantically
- Everything gets easier
What's not to like? 😃
Thanks a lot @mhsdesign!
|
:D |
... instead of multiple operands
i inlined $left and $right into the BinaryOperationNode:
this will make working with the BinaryOperandNode (eg type resolving/narrowing) much easier.
Also the php and js AST also use the fields $left $right and $operator instead of a collection (even when the same operator is used).
as a sideeffect we now also transpile
true === true === truecorrectly to php (by wrapping it in parenthesis):(true === true) === truewithout parenthesis it is not valid php (its valid ecmascript though ^^)
The above mentioned case with
=== trueis obviously an edge case and currently1 + 1 + 1is also transpiled with parenthesis:(1 + 1) + 1which are optional and not needed by php - we still need to decide if we like this see #16 (comment).