Conversation
|
I'll take a look in detail ASAP. You'll have to update the paths/imports unfortunately since I just changed it all last night based on the entire discussion we had in the Rust channel... But it should be pretty easy and you can just copy-paste |
gtars v0.5.0 release (as workspace)
|
One difference from the pyo3 bindings is that you can't build R S4 classes directly in rust, and so conceptually the R bindings work a little differently for object-related things. In GlobalRefgetStore is a more complex object with its own rust methods, and since you can't define R S4 classes and methods in rust like pyclass and pymethods, AI suggested writing a wrapper R class and using an external pointer to a rust object in memory. So in |
nleroy917
left a comment
There was a problem hiding this comment.
I think these are good. Im curious about those unsafe pointers... but I think its ok for now. Especially since we aren't really distributing this even yet.
There was a problem hiding this comment.
I think we should be gitignore-ing and __pycache__ dirs
gtars-r/LICENSE
Outdated
| @@ -0,0 +1,2 @@ | |||
| YEAR: 2024 | |||
| read_tokens_from_gtok(&filename) | ||
| .unwrap() | ||
| .into_iter() | ||
| .map(|gtok| gtok as i32) |
There was a problem hiding this comment.
is i32 necessary for R? Otherwise, a u32 makes more sense here
| let store_raw_ptr = unsafe { store_ptr.external_ptr_addr::<GlobalRefgetStore>() }; | ||
| if store_raw_ptr.is_null() { | ||
| return Err("Invalid store pointer".into()); | ||
| } |
There was a problem hiding this comment.
I see. This is the pointer stuff... is this how rextendr recommends it? I think this is probably fine honestly. I'm much more concerned with unsafe in the core crates...
There was a problem hiding this comment.
Briefly, are we able to explain why this is necessary? Can we not point to a file on disk instead? Im sure there's a reason... but curious
|
Just make sure you're in sync with the latest master! |
Created R refget bindings that function similarly to the python bindings