-
Notifications
You must be signed in to change notification settings - Fork 151
Remove r_env_find() and r_env_find_anywhere()
#1872
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
Conversation
c409217 to
32d4145
Compare
DavisVaughan
left a comment
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.
A few notes from our offline conversation for future us:
-
r_env_get()has always triggered active bindings, due to howRf_findVarInFrame3()also triggers active bindings. This feels right to us. If you need to specially handle active, delayed, or forced bindings then you should switch on the binding type up front. -
An emerging pattern is moving from a single
r_env_find()call tor_env_has()+r_env_get() -
It's unfortunate that
R_getVar()errors onR_MissingArgbindings. This is a valid symbol to bind into an environment, and it feels like a shame that you can't pluck it out of there as well. It's certainly not "unbound". It feels like onlyRf_eval()should care about erroring onR_MissingArg. IfR_getVar()allowedR_MissingArg, thenr_env_get()could have just used that directly, but without that it feels like we will never be able to make this switch.
| r_obj* promise = KEEP(Rf_allocSExp(PROMSXP)); | ||
| SET_PRCODE(promise, expr); |
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.
You were KEEP()ing expr here before the allocation, I think we probably want that?
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.
hmm rchk wouldn't be happy with that (relying on an internal protect), and it seems weird to protect expr but not other arguments?
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.
oh wait regarding rchk I think I'm confusing something else, i.e. that the argument is considered protected after the scope has finished.
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.
It looks like in R_MakeDelayedBinding we aren't protecting anything directly
https://github.com/r-devel/r-svn/pull/206/changes#diff-5272e85b221848a45cc500d6792e9aba179ddbbaa817e48757b686deead11245R3502
But Rf_mkPROMISE protects both expr and evalEnv internally.
Then for R_MakeForcedBinding it looks like that drops into R_mkEVPROMISE which only protects the expr and not the value via an internal call to mkPROMISE (which we determined protects expr)
https://github.com/r-devel/r-svn/blob/f98b44aeb0f6b28891d5b1febfdaea52c2a5647a/src/main/memory.c#L2646
This is quite challenging to wrap my head around
|
|
||
| r_obj* promise = r_env_find(env, sym); | ||
| SET_PRVALUE(promise, value); | ||
| r_obj* promise = KEEP(Rf_allocSExp(PROMSXP)); |
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.
Same comment about KEEP(expr), and idk if we want to KEEP(value) too or not?
I don't remember what we made R_MakeForcedBinding did
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 an assignment operation can't protect two values at the same time because that would mean that the first temporary object would be unprotected while the second is created in:
bind(x, foo(), bar())f0f7cc7 to
f28891d
Compare
|
We decided to embrace the explicit protection pattern when assigning. Before: obj_set(x, new());After: obj_set(x, KEEP(new());
FREE(1);The need to free after the assignment is especially annoying, but there's no way to implement a fully general assignment principle without getting into rchk complications. And the principle breaks down if more than one object are assigned. And without the explicit protection, there's always a small doubt in the back of the head when reading code compelling you to double check what the assignment does. |
Follow up to #1871