Update environment binding API with planned ctors and accessors #1871
Conversation
lionel-
force-pushed
the
feature/c-env-api
branch
from
1cc1f28 to
9dcf9dc
Compare
DavisVaughan
left a comment
DavisVaughan
left a comment
There was a problem hiding this comment.
Looks like a nice API to me!
| r_obj* value = r_env_find(env, sym); | ||
|
|
||
| if (value == r_syms.unbound) { | ||
| return R_ENV_BINDING_TYPE_unbound; | ||
| } | ||
|
|
||
| if (value == r_missing_arg) { | ||
| return R_ENV_BINDING_TYPE_missing; | ||
| } | ||
|
|
||
| if (r_typeof(value) == R_TYPE_promise) { |
There was a problem hiding this comment.
I wouldn't be against implementing these in terms of just pure R API, rather than r_env_find(). That way we could fully remove r_env_find() eventually without having to think about "old R" support
There was a problem hiding this comment.
hmm I might be misunderstanding but I'm not sure how that would work. I'm fine having the old R support in place, this code will likely be unchanged for years once the R API has stabilised.
There was a problem hiding this comment.
all i meant here was using Rf_findVarInFrame3() directly in place of r_env_find(), in case we wanted to remove r_env_find()
| r_env_unbind(env, sym); | ||
| R_MakeActiveBinding(sym, fn, env); |
There was a problem hiding this comment.
Interesting that R_MakeActiveBinding() fails on preexisting standard bindings
| r_abort("Not a forced binding."); | ||
| } | ||
|
|
||
| return PRVALUE(value); |
There was a problem hiding this comment.
I faintly remember us saying that you are supposed to just Rf_eval() a forced binding to get its value, which is why we didn't add R_ForcedBindingValue(). Maybe use that?
There was a problem hiding this comment.
We have the promise already so doing a separate lookup might be surprising in other ways
There was a problem hiding this comment.
But PRVALUE() is non-API. So what's the correct API compliant way to get the value?
| KEEP(value); | ||
| Rf_defineVar(sym, value, env); | ||
| FREE(1); |
There was a problem hiding this comment.
From what I can tell, Rf_defineVar() will protect value if it needs to, so I think you can skip this
There was a problem hiding this comment.
I'm not sure but rchk might take issue with that, I faintly remember having to do macros to get it to acknowledge the protection.
src/rlang/env-binding.h
Outdated
| } | ||
|
|
||
| static inline | ||
| void r_env_poke(r_obj* env, r_obj* sym, r_obj* value) { |
There was a problem hiding this comment.
For legacy reasons? Seems like we use it a lot
There was a problem hiding this comment.
I missed that, I have now renamed it to r_env_bind()
| r_env_binding_delayed_expr(from, sym), | ||
| r_env_binding_delayed_env(from, sym) | ||
| ); | ||
| break; | ||
|
|
||
| case R_ENV_BINDING_TYPE_forced: | ||
| r_env_bind_forced( | ||
| env, | ||
| sym, | ||
| r_env_binding_forced_expr(from, sym), | ||
| r_env_binding_forced_value(from, sym) |
There was a problem hiding this comment.
I guess this relies on the _expr, _env, and _value helpers never allocating, right? Because if r_env_binding_delayed_expr() and r_env_binding_delayed_env() allocate then we'd have a protection issue. But I guess we'd say that the promise is protecting them?
There was a problem hiding this comment.
I think you're right, this relies on the assumption that the accessors do pure access and don't allocate. Otherwise rchk will flag us down.
DavisVaughan
left a comment
DavisVaughan
left a comment
There was a problem hiding this comment.
Looks like a nice API to me!
2 participants
So we can write algorithms that can be easily switched to the API discussed at https://bugs.r-project.org/show_bug.cgi?id=18928