508 validation for assigning output variables not working #616
Conversation
VariableIndexEntries now offer a SymbolLocation that consists of a line-nr, and offset, and a file-name
* adds failing test * add missing test attribute * PLC-lang#593 fix wrong test * PLC-lang#593 case condition validation * PLC-lang#593 remove comment * PLC-lang#593 remove prints Co-authored-by: Michael HASELBERGER <Michael.HASELBERGER@bachmann.info> Co-authored-by: Ghaith Hachem <ghaith.hachem@gmail.com>
…ype (PLC-lang#613) * Fix Enum resolution * Add test for resolver
VariableIndexEntries now offer a SymbolLocation that consists of a line-nr, and offset, and a file-name
…ype (PLC-lang#613) * Fix Enum resolution * Add test for resolver
| @@ -2626,13 +2631,14 @@ impl<'ink, 'b> ExpressionCodeGenerator<'ink, 'b> { | |||
| /// If the parameter is already implicit, it does nothing. | |||
| /// if the parameter is explicit ´param := value´, | |||
| /// it returns the location of the parameter in the function declaration | |||
| /// as well as the parameter value (right side) ´param := value´ => ´value´ | |||
| /// as well as the parameter value (right side) ´param := value´ => ´value´ | |||
| /// and `true` for implicit / `false` for explicit parameters | |||
There was a problem hiding this comment.
This this method is called "get_implicit" it's strange to return a boolean saying if the return value is actually implicit.. it should always be.
I would prefer having a new function "is_implicit" that returns that info (Should be a match on Assignment/Output Assingment with no further checks
There was a problem hiding this comment.
the idea was to safe another check when it is exactly what we are doing in this function
i would maybe change the name to get_call_parameter we pass explicit and implicit parameters to the function ?
if not im ok if we make another function is_implicit
| @@ -40,11 +41,11 @@ entry: | |||
|
|
|||
| define void @main(%main* %0) { | |||
| entry: | |||
| %1 = alloca i32, align 4 | |||
| store i32 1, i32* %1, align 4 | |||
| %1 = alloca i16, align 2 | |||
There was a problem hiding this comment.
why would the type change here?
There was a problem hiding this comment.
function declaration
FUNCTION func : DINT
VAR_INPUT {ref}
byRef1 : INT;
...
function call
func(1,2,3,4);
before we passed 1 as i32 it should be i16
There was a problem hiding this comment.
this change is no longer in this PR
i still think we should generate an i16
but i would create a new issue for this
Codecov ReportBase: 93.70% // Head: 93.73% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #616 +/- ##
==========================================
+ Coverage 93.70% 93.73% +0.03%
==========================================
Files 46 46
Lines 17461 17530 +69
==========================================
+ Hits 16361 16431 +70
+ Misses 1100 1099 -1
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. |
There was a problem hiding this comment.
👍 good implementation - I suggested some minor improvements which are kind of tricky. The decision to not change the semantics of the get_intrinsic_type method is hard to explain - it feels not quite right for me - but I think in general you were on the right track.
we can talk this through if you have some questions!
| .index | ||
| .get_effective_type_or_void_by_name(param.get_type_name()) | ||
| }); | ||
| let right_type = context.ast_annotation.get_type(right, context.index); |
There was a problem hiding this comment.
I think we need to check against context.ast_annotation.get_type_hint(right, context.index).or_else(|| context.ast_annotation.get_type(...)); because the annotator may already requested a cast which will be reflected by the type-hint.
--> and we should get a helper in the annotation-struct since we do this again and again.
There was a problem hiding this comment.
we will always annotate if the cast can be done or not
-> trying to pass a string to int parameter -> we will have an annotation to the int type therefore we will always have the same type and never get the correct validation
…r-assigning-output-variables-not-working
99NIMI
deleted the
508-validation-for-assigning-output-variables-not-working
branch
No description provided.