-
Notifications
You must be signed in to change notification settings - Fork 55
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
feat: Introduce REF=
and REFERENCE_TO
#1251
Conversation
REFERENCE_TO
REF=
and REFERENCE_TO
…into volsa/referenceto
1ac7bfd
to
7772a3b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, but I have some open questions/requests for additional test coverage.
b60bc18
to
c5680bd
Compare
I think I addressed all your comments @mhasel , should be ready for a second round 🤞 |
* `foo : ARRAY[...] OF REFERENCE TO (* ... *)` | ||
* `foo : REF_TO REFERENCE TO (* ... *)` | ||
|
||
Furthermore `REFERENCE_TO` variables must not be initialized in their declaration, e.g. `foo : REFERENCE TO DINT := bar`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi, this is not entirely correct but will be addressed in the aliasing PR later on
foo REF= bar; | ||
|
||
// CHECK-NOT: ^5$ | ||
printf('%d$N', foo); // We expect some random value here because foo isn't pointing to any variable yet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are auto-dereferencing an uninitialized pointer here - this is undefined behaviour. Should we initialize these pointers with null
like we are doing for VAR_INPUT {ref}
? Ideally we would check for use before assignment but that feature feels a bit far off still.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline, generally in favor of NULL pointers here but I'd probably also address this in another PR. To have a second opinion here though cc @ghaith
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think null makes sense here so that we don't have random values. I'm in favour of not having such tests tbh. We can't test undefined behaviour (by design). Also what happens if the random value is 5? I am more in favour of having a test that verifies the pointer value of foo is null. Not sure how to do this with the auto deref rules.. maybe it's enough to have an IR check that verifies the initial values of REFERENCE TO and Pointer fields in general.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I've created an issue and deleted the test here. I'll address it in another PR since this one bloats up way too much if you'd ask me.
This PR introduces two new keywords, namely
REF=
andREFERENCE TO
:REF=
is essentially syntactic sugar for an assignment where the right-hand side is wrapped in aREF()
function call. Thereforefoo := REF(bar)
andfoo REF= bar
are equivalent.REFERENCE TO
is identical toREF_TO
with the exception of being auto-deref by default. A variablefoo
declared asREFERENCE TO
will therefore auto-deref on assignments, i.e.foo := 5
is equivalent tofoo^ := 5
.More information on CodeSys' REF= and REFERENCE TO documentation pages.