-
Notifications
You must be signed in to change notification settings - Fork 139
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
Introduce dereference operator for references #2982
Conversation
Need to cover a few more types in the interpreter tests. |
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.
Nice!
CI is reporting data races: https://github.com/onflow/cadence/actions/runs/7240009930/job/19722511487?pr=2982 |
Co-authored-by: Daniel Sainati <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2982 +/- ##
==========================================
- Coverage 80.15% 80.00% -0.16%
==========================================
Files 348 356 +8
Lines 81989 83037 +1048
==========================================
+ Hits 65716 66431 +715
- Misses 13952 14247 +295
- Partials 2321 2359 +38
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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, great work!
In general I'm still not sure if this should be a magic function on references, or rather something else, like an operator.
For the functionality to be a magic function, the PR should also handle the case where a composite declares a member named the same as the magic function, as it seems like the current implementation shadows / makes it inaccessible. We might want to forbid declaring members which have that magic name. An operator would not have that problem.
We might want to use an operator here instead of / in addition to the method, e.g. the unary prefix operator |
Sure, will update to remove the function and just have the operator. |
@darkdrag00nv2 Not sure if you started yet, but if not: I'm half-way done and could finish and push it up here |
@turbolent Haven't started yet. You can push it. Thank you! |
dereference
on the Reference type
Refactored the @onflow/cadence could you please have a look? |
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.
Now that dereference
isn't a member function, it would be nice for *
to have simple/clean behavior with respect to optional references. I.e. if we have a &Int?
type, it would be nice for the *
operator on that type to produce a Int?
. Previously we could do this with ?.
chaining, but with the change to an operator we lose this functionality. Can we add this (and tests for it)?
@dsainati1 |
Yeah, it would be nice for the |
@dsainati1 Sounds good! Let's get that added in a follow-up PR. Could you please open a feature request issue for it? |
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.
LGTM!
Closes #2803
Description
master
branchFiles changed
in the Github PR explorer