Skip to content
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

fix(parser): make if statements with 'not' work #443

Merged
merged 5 commits into from
Jul 1, 2020

Conversation

lkipke
Copy link
Collaborator

@lkipke lkipke commented Jun 11, 2020

Change Summary

This allows if statements with a logical not operator, and closes #329.

@lkipke lkipke added bug Any difference between this BrightScript implementation and RBI, or otherwise unexpected behavior parser Affects this project's token parser labels Jun 11, 2020
@lkipke lkipke self-assigned this Jun 11, 2020
Copy link

@Piera Piera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to get my head around this -- had a couple of questions. 🤔

test/e2e/resources/conditionals.brs Show resolved Hide resolved
test/e2e/resources/conditionals.brs Show resolved Hide resolved
Copy link

@Piera Piera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates!

@@ -1316,7 +1316,7 @@ export class Parser {
function prefixUnary(): Expression {
if (match(Lexeme.Not, Lexeme.Minus)) {
let operator = previous();
let right = prefixUnary();
let right = relational();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Considering the order of operations goes relational > bitshift > additive > multiplicative > exponential > prefixUnary, I was going to add some unit tests that did things like not 2 * 3. However, that elicits strange behavior on RBI (not 2 * 3 = -7 for some reason...) and brs prevents comparing boolean NOT to non-boolean 2 * 3.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 interesting!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, seems like there's still some bugs in brs that we'll need to shake out then! From RBI:

print not  0 ' => -1
print not  1 ' => -2
print not -1 ' =>  0
print not  2 ' => -3
print not -2 ' =>  1

which looks like RBI is using a two's complement representation for numbers and does something slightly unexpected with it. not someInteger in RBI seems to take someInteger, convert to a binary representation, perform a bitwise negation on it (all 0s become 1s, all 1s become 0s), then convert back to a decimal representation in the Two's complement understanding. So for not 0 (using just 4-bits for now because I'm lazy), we have:

not 0 (decimal) == not 0b0000 == 0b1111 == -1 (decimal in two's complement)

How strange! In the not 2 * 3 example, we get:

not (2 * 3) == not 6 (decimal) == not 0b0110 == 0b1001 == -7 (decimal in two's complement)

All-up, it looks like RBI supports negating decimals, but brs doesn't! I'll file a bug to fix that a bit later. At least the parser is still correct 🎉

Copy link
Owner

@sjbarag sjbarag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very exhaustive tests! Thanks for including them here 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Any difference between this BrightScript implementation and RBI, or otherwise unexpected behavior parser Affects this project's token parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IF statement with NOT before a logic comparison expression shows an error
3 participants