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

constant propagation support #504

Conversation

riederm
Copy link
Collaborator

@riederm riederm commented Jun 19, 2022

when generating an expression we check if this may refer to a constant
expression and rather generate it's initial value instead of a
reference to the global variable.

fixes #391 and #291

@codecov-commenter
Copy link

codecov-commenter commented Jun 19, 2022

Codecov Report

Merging #504 (f91c8df) into master (679ffe8) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head f91c8df differs from pull request most recent head c6a7147. Consider uploading reports for the commit c6a7147 to get more accurate results

@@            Coverage Diff             @@
##           master     #504      +/-   ##
==========================================
- Coverage   93.01%   93.01%   -0.01%     
==========================================
  Files          40       40              
  Lines       14428    14468      +40     
==========================================
+ Hits        13420    13457      +37     
- Misses       1008     1011       +3     
Impacted Files Coverage Δ
src/codegen/generators/expression_generator.rs 87.93% <100.00%> (+0.22%) ⬆️
src/index.rs 97.46% <100.00%> (+0.01%) ⬆️
src/index/const_expressions.rs 88.23% <100.00%> (+0.73%) ⬆️
src/tests/adr/enum_adr.rs 100.00% <100.00%> (ø)
src/codegen/generators/variable_generator.rs 93.33% <0.00%> (-1.12%) ⬇️
src/codegen/llvm_index.rs 97.27% <0.00%> (-0.91%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 679ffe8...c6a7147. Read the comment docs.

when generating an expression we check if this may refer to a constant
expression and rather generate it's initial value instead of a
reference to the global variable.

fixes #391 and #291
@riederm riederm force-pushed the 391-const-references-in-case-statements-not-supported-segfault-during-compilation branch from 01f0e53 to 85e7514 Compare June 19, 2022 20:21
@riederm riederm requested a review from ghaith June 19, 2022 21:24
Copy link
Collaborator

@ghaith ghaith left a comment

Choose a reason for hiding this comment

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

Looks good, but could you add a correctness test for the switch case as well?
Also one using constants in body would be great

testcases:
- constants & const-expressions used as initial values
- constants & const-expressions used in case-statement
- constants & const-expressions used in array-declaration
@riederm
Copy link
Collaborator Author

riederm commented Jun 20, 2022

Looks good, but could you add a correctness test for the switch case as well? Also one using constants in body would be great

done - added correctness tests

@riederm riederm requested a review from ghaith June 20, 2022 21:31
gF : LREAL := cF;
END_VAR

PROGRAM main : DINT
Copy link
Collaborator

Choose a reason for hiding this comment

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

Programs don't return

let src = r#"
VAR_GLOBAL CONSTANT
cI : DINT := 2 * 5;
cB : BOOL := (CI-5 = 5);
Copy link
Collaborator

Choose a reason for hiding this comment

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

case difference CI != cI

@ghaith ghaith merged commit 0163281 into master Jun 21, 2022
@ghaith ghaith deleted the 391-const-references-in-case-statements-not-supported-segfault-during-compilation branch June 21, 2022 08:41
riederm added a commit that referenced this pull request Jun 21, 2022
added a test that shows that constants are propagated even in
ranged-type's limits, so even if you use (const-)expressions (a+b)
to define the limits of a ranged type, the const-expressions
will be evaluated during compile time and the result will be used.
- as in contrast we would do this calculation all the time at
runtime.

the behavior was introduced in PR #504 (016328)

fixes #350
riederm added a commit that referenced this pull request Jun 21, 2022
added a test that shows that constants are propagated even in
ranged-type's limits, so even if you use (const-)expressions (a+b)
to define the limits of a ranged type, the const-expressions
will be evaluated during compile time and the result will be used.
- as in contrast we would do this calculation all the time at
runtime.

the behavior was introduced in PR #504 (016328)

fixes #350
ghaith pushed a commit that referenced this pull request Jun 22, 2022
added a test that shows that constants are propagated even in
ranged-type's limits, so even if you use (const-)expressions (a+b)
to define the limits of a ranged type, the const-expressions
will be evaluated during compile time and the result will be used.
- as in contrast we would do this calculation all the time at
runtime.

the behavior was introduced in PR #504 (016328)

fixes #350
@ghaith ghaith mentioned this pull request Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

const-references in case-statements not supported (Segfault during compilation)
3 participants