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
| /// 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
| 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. |
riederm left a comment
riederm left a comment 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 4 participants
No description provided.