Add safe r_length() function
#23
Conversation
|
I have a slight preference for using a signed integer as the return value because in C code I somewhat frequently initialize a |
|
That makes sense. I'll revert to using #[doc = "R_xlen_t is defined as int on 32-bit platforms, and\n that confuses Rust. Keeping it always as ptrdiff_t works\n fine even on 32-bit.\n <div rustbindgen replaces=\"R_xlen_t\"></div>"]
pub type R_xlen_t = isize; |
There was a problem hiding this comment.
In general I have a strong preference for always using signed sizes to avoid coercions with mixed type arithmetic (less of an issue in Rust?) and the surprising behaviour when substracting two unsigned types.
See discussion in https://stackoverflow.com/a/34642843/1725177 and Bjarne Stroutrup's advice in https://www.youtube.com/watch?v=Puio5dly9N8:
- Never make signed an unsigned.
- You don't need the extra bit.
- It doesn't matter that something can't be negative.
| @@ -19,6 +19,7 @@ use harp::utils::r_is_data_frame; | |||
| use harp::utils::r_is_matrix; | |||
| use harp::utils::r_is_simple_vector; | |||
| use harp::utils::r_typeof; | |||
| use harp::utils::r_xlength; | |||
There was a problem hiding this comment.
I think this could be called r_usize()? (or r_ssize() if signed, as in the rlang C lib - edit: oh I see from your comment that this is r_isize() in rust)
There was a problem hiding this comment.
actually it's r_length() in rlang, sorry for the confusion
r_ssize r_length(r_obj* x);There was a problem hiding this comment.
We might want to keep _size() for size of object in bytes: posit-dev/positron#478
romainfrancois
changed the title
r_length() function that returns usize r_length() function
|
@DavisVaughan should we close this one, since it overlaps with #26 ? This PR was mostly a follow up to this comment posit-dev/positron#354 (comment) |
|
Yea I think if it is ok with you, I'll probably end up adding it with or as a follow up to #26 |
Follow up from posit-dev/positron#354 (comment)
I think usize is the right return although it causes some casting to be needed here and there