From<??> for RObject and PartialEq<??> for RObject where ?? is a slice of vector of strings (&str or String)

[romainfrancois]

Now only borrows, i.e. impl From<&Vec<String>> for RObject and correctly constructs the CHARSXP (this is what dragged me here in the first place).

Or did we really mean for the passed Vec to be dropped ?

[kevinushey]

By convention, the From trait is used to implement "consuming value-to-value" conversions. See for example:

https://doc.rust-lang.org/std/convert/index.html
https://doc.rust-lang.org/std/convert/trait.From.html

For that reason, I think the original implementation is still preferred here.

[kevinushey]

I wonder if this could be implemented as a trait / method on the RObject class, so one could write:

let object = RObject::new();
object.equals(something);

and then we'd have separate implementations for the various kinds of objects we care about. This would keep the API a little bit cleaner, I think?

[kevinushey]

If that seems like a reasonable approach, we could add something here:

https://github.com/rstudio/positron/blob/7593f9899f0d7e721b2cb0fbce4f54e312dec5a8/extensions/positron-r/amalthea/crates/harp/src/object.rs#L109-L111

and then have various implementations of that trait for different RHS objects.

[romainfrancois]

I'd say we can sit on this for now, perhaps we'll want to have dedicated rust classes instead of a catch all RObject i.e. the same way we have cpp11::strings They could have dedicated traits

[romainfrancois]

I went another direction so that we can assert_eq!() an RObject with strings, with various "strings"

fn test_RObject_from_Vec_str() { r_test! {
        let expected = ["Apple", "Orange", "한"];

        // RObject from Vec<&str>
        let r_strings = RObject::from(expected.to_vec());
        assert_eq!(r_strings, expected);              // [&str]
        assert_eq!(r_strings, expected[..]);          // [&str; const N]
        assert_eq!(r_strings, expected.to_vec());     // Vec<&str>

        assert_eq!(expected         , r_strings);     // [&str]
        assert_eq!(expected[..]     , r_strings);     // [&str; const N]
        assert_eq!(expected.to_vec(), r_strings);     // Vec<&str>
    }}

[romainfrancois]

By convention, the From trait is used to implement "consuming value-to-value" conversions

Thanks. I'll rewind that part of the PR then, it makes sense. My thinking was "this is going to create new strings anyway, so we might as well keep the old value".

[kevinushey]

I believe you want CStr rather than CString here, as the memory for the string is "owned" by the Rust vector.

[romainfrancois]

Using something different with:

impl ToCharSxp for &str {
    fn to_charsxp(&self) -> SEXP {
        unsafe {
            Rf_mkCharLenCE(self.as_ptr() as *mut c_char, self.len() as i32, cetype_t_CE_UTF8)
        }
    }
}

impl ToCharSxp for String {
    fn to_charsxp(&self) -> SEXP {
        (&self[..]).to_charsxp()
    }
}

The problem usually is that self.as_ptr() is not null terminated, but Rf_mkCharLenCE() does not care about that.

[kevinushey]

The problem usually is that self.as_ptr() is not null terminated, but Rf_mkCharLenCE() does not care about that.

Interesting -- I guess R is null terminating the string for us? I was curious whether that was true and it looks like it happens here:

https://github.com/wch/r-source/blob/22453c94794b946bc46a48bd1c379d0f8f9c5f1b/src/main/memory.c#L2701-L2705

Presumedly R is also allocating and zeroing the memory somewhere as well, but I got dizzy after scrolling further down...

[kevinushey]

LGTM; nice work!

For now, let's leave other extensions to harp until we know we need them; e.g. I'm sure we'll want some subset of methods when we think about how to summarize or describe an object in short form but let's implement them as we find they're required -- we don't need a "complete" API at this point, we can build it as we go.

[kevinushey]

The problem usually is that self.as_ptr() is not null terminated, but Rf_mkCharLenCE() does not care about that.

Interesting -- I guess R is null terminating the string for us? I was curious whether that was true and it looks like it happens here:

https://github.com/wch/r-source/blob/22453c94794b946bc46a48bd1c379d0f8f9c5f1b/src/main/memory.c#L2701-L2705

Presumedly R is also allocating and zeroing the memory somewhere as well, but I got dizzy after scrolling further down...

[romainfrancois]

For now, let's leave other extensions to harp until we know we need them

Definitely. I'm very conscious about the risk of spending too much time crafting the perfect seamless harp and then never playing it.

I'll come back to it as needs materialize higher up.