Implement is not and not in operators - #454
Conversation
| result.setText(null); | ||
| } else { | ||
| // The tokens straddle an include file boundary, so there is no single source span to read. | ||
| StringBuilder text = new StringBuilder(); |
There was a problem hiding this comment.
Since we know the exact size of the string, let's set the StringBuilder capacity here:
| StringBuilder text = new StringBuilder(); | |
| StringBuilder text = new StringBuilder(count); |
There was a problem hiding this comment.
All these changes to support NOT IN in the ExpressionParser look good, but IS and IS NOT also need to be supported - they are valid in compiler directives, though seem to blindly return true. The following compiles and runs, and prints both lines.
program foo;
var
Obj: TObject;
begin
{$if Obj is TObject}
WriteLn('is');
{$endif}
{$if Obj is not TObject}
WriteLn('is not');
{$endif}
ReadLn;
end.There was a problem hiding this comment.
I didn't expect this at all since it makes no real sense at compile-time, which is why is isn't supported to begin with.
I'll take a closer look.
There was a problem hiding this comment.
This was a very neat little nightmare that forced me to split unevaluable expressions into 2 categories based on a mix of compiler behavior probing and respecting the SonarDelphi canon (fanfiction preprocessor implementation).
There was a problem hiding this comment.
Oh, and I also randomly found out that we weren't handling string ordering comparisons, so that's been fixed in a separate commit as well.
| - **API:** `DelphiTokenType.IS_NOT` token type. | ||
| - **API:** `DelphiTokenType.NOT_IN` token type. | ||
| - **API:** `BinaryOperator.IS_NOT` enum value. | ||
| - **API:** `BinaryOperator.NOT_IN` enum value. |
There was a problem hiding this comment.
Changelog should also include the addition of BinaryExpressionNode::getOperatorNode().
| @Override | ||
| public DelphiCheckContext visit(BinaryExpressionNode node, DelphiCheckContext context) { | ||
| if (node.getOperator() == BinaryOperator.IN) { | ||
| if (node.getOperator() == BinaryOperator.IN || node.getOperator() == BinaryOperator.NOT_IN) { |
There was a problem hiding this comment.
I don't know if this is a necessary change... This rule is making up for if not A in B then looking correct to people without a strong knowledge of Delphi precedence. I don't think if not A not in B then really has the same problem - to me that actually does read correctly as a bitwise not followed by a logical not. It's sort of a different class of mistake and including it in this rule muddies the waters a little bit.
There was a problem hiding this comment.
Reasonable take, I agree and will back this one out.
Delphi treats a non-constant `{$IF}` condition expression as true, but
we were treating every unevaluable expression as false.
Unevaluable expressions now come in 2 flavors:
- `UNKNOWN` for badly-formed expressions or unresolvable references.
Evaluates to false.
- `NON_CONSTANT` for well-formed non-constant expressions.
Evaluates to true.
String ordering comparisons like `'foo' > 'bar'` previously evaluated to an UNKNOWN expression in preprocessor directive expressions.
In terms of parsing, operator precedence, and type resolution, this wasn't a difficult implementation. There were other challenges. "Compound" operators with multiple tokens make previous AST modeling of binary and unary expression nodes a little tricky. The operators in these expressions are now modeled as their own typed child nodes. This required me to build binary expressions a little differently, which made me drop the `resetBinaryExpressionTokens` fix introduced in 041a0d9. In doing so, I had to wrestle with node text range calculation for (probably) the 27th time. The new shape seems to work well.
4eecf1d to
d933342
Compare
2 participants
This PR implements the new binary operators added in Delphi 13.
In terms of parsing, operator precedence, and type resolution, this wasn't a difficult implementation. There were other challenges.
"Compound" operators with multiple tokens make previous AST modeling of binary and unary expression nodes a little tricky. The operators in these expressions are now modeled as their own typed child nodes.
This required me to build binary expressions a little differently, which made me drop the
resetBinaryExpressionTokensfix introduced in 041a0d9. In doing so, I had to wrestle with node text range calculation for (probably) the 27th time. The new shape seems to work well.Closes #408, #409.