Skip to content

Support literal numbers with explicit plus sign (#633)#640

Merged
riederm merged 4 commits intoPLC-lang:masterfrom
volsa:633-literal-numbers-with-plus-sign
Nov 10, 2022
Merged

Support literal numbers with explicit plus sign (#633)#640
riederm merged 4 commits intoPLC-lang:masterfrom
volsa:633-literal-numbers-with-plus-sign

Conversation

@volsa
Copy link
Member

@volsa volsa commented Nov 8, 2022

Hi, #633 looked like a beginner friendly issue so I thought I'd give it a shot; the PR should be hopefully OK :)

volsa added 2 commits November 8, 2022 19:43
Fixes issue #633 where previously the compiler would yield an error if it encountered a number with an explicit plus sign, i.e. the following was previously not supported but is now ``` foo : DINT := +2147483647; ```
@codecov-commenter
Copy link

codecov-commenter commented Nov 9, 2022

Codecov Report

Base: 93.76% // Head: 93.76% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (87b6040) compared to base (35842fa).
Patch coverage: 76.47% of modified lines in pull request are covered.

Additional details and impacted files
@@ Coverage Diff @@ ## master #640 +/- ## ======================================= Coverage 93.76% 93.76% ======================================= Files 46 46 Lines 17604 17620 +16 ======================================= + Hits 16506 16522 +16  Misses 1098 1098 
Impacted Files Coverage Δ
src/parser/expressions_parser.rs 94.60% <76.47%> (+0.11%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@riederm riederm left a comment

Choose a reason for hiding this comment

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

hi @volsa, thx for the PR!
the check-style-step of the build failed, you can fix the formatting via: cargo fmt

PROGRAM exp
x : DINT := 1;
y : DINT := +1;
END_PROGRAM
Copy link
Collaborator

Choose a reason for hiding this comment

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

this ST-code is not 100% correct. it should maybe look like this:

PROGRAM exp VAR x : INT; END_VAR x := 1; x := +1; END_PROGRAM
Copy link
Member Author

Choose a reason for hiding this comment

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

Out of curiosity, would the following ST-code also be wrong because it's missing a VAR block?

PROGRAM exp x := NULL; END_PROGRAM

(taken from the assignment_to_null() test)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Out of curiosity, would the following ST-code also be wrong because it's missing a VAR block?

PROGRAM exp x := NULL; END_PROGRAM

(taken from the assignment_to_null() test)

so technically this works, if x is declared somewhere as a global variable. When we do parse tests, we often dont have semantically correct cases: (e.g. we dont care if x would resolve to something meaningful or not).
your sourcode mixed a statement and a declaration, you cannot have declarations outside of VAR-blocks.

I was really surprised that the parser actually returned something 😄 . but if you look at your assignment, the parser created something very strange:
Assignment{ left: "DINT", right: "1" }

so the parser thought that DINT is an identifer to a variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that makes sense, thanks for clarifying.

@riederm
Copy link
Collaborator

riederm commented Nov 9, 2022

I did some experiments and it looks like this code only covers behavior for literals. Can we also fix & test the behavior for reference?

like: ? x := +x;. I know it does nothing, but we should be able to parse it. I think this would just require a small tweak in

fn parse_unary_expression(lexer: &mut ParseSession) -> AstStatement {
let operator = match lexer.token {
OperatorNot => Some(Operator::Not),
OperatorMinus => Some(Operator::Minus),
OperatorAmp => Some(Operator::Address),
_ => None,
};
to also allow the plus sign.

@volsa
Copy link
Member Author

volsa commented Nov 9, 2022

Done! Fingers crossed the style-checker doesn't throw an error :P

Copy link
Collaborator

@riederm riederm left a comment

Choose a reason for hiding this comment

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

very nice, thx! 👍

@riederm riederm merged commit 3a76564 into PLC-lang:master Nov 10, 2022
@riederm riederm mentioned this pull request Nov 10, 2022
@volsa volsa deleted the 633-literal-numbers-with-plus-sign branch November 10, 2022 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants