libR-sys icon indicating copy to clipboard operation
libR-sys copied to clipboard

Replace `SEXPREC` with something truly opaque

Open CGMossa opened this issue 2 years ago • 7 comments

Replacement for #205, with a pattern from Rust for Rustacean book, see https://rust-for-rustaceans.com/#errata

This is a far more complete approach

  • No longer deriving Copy or Clone for something we cannot construct, so how would we copy/clone it?
  • Using c_void to indicate that we don't know anything at all about it, but still have a typed SEXP.
  • Even if we have access to SEXPREC, in the defining module, we cannot construct it in this definition, while the zero-sized arrays could potentially be constructed.

@yutannihilation What do you think?

Depends on

  • [ ] #208

CGMossa avatar Nov 18 '23 17:11 CGMossa

My comment stays the same as https://github.com/extendr/libR-sys/pull/205#issuecomment-1817496032; I believe there's no need to expose SEXPREC at all, but I'm approving.

yutannihilation avatar Nov 20 '23 00:11 yutannihilation

I see. To be honest with you, I felt very proud of this PR, because I thought I basically did exactly what you asked.

I can now see the subtle difference in what you said and what I did here.

I honestly like this. I followed what was presented in the issue I created over at bindgen repo.

CGMossa avatar Nov 20 '23 17:11 CGMossa

I get you. But, my understanding is that, the comments on https://github.com/rust-lang/rust-bindgen/issues/2685 is probably under the assumption that other APIs use SEXPREC* in the signatures because it's a common case. Actually, this example in Rustonomicon is the same:

struct Foo; /* Foo is a structure, but its contents are not part of the public interface */
struct Bar;
void foo(struct Foo *arg);
void bar(struct Bar *arg);

In this case, the Rust bindings should provide Foo and Bar. Apparently, the things are different in the R API. All the signatures use a type alias SEXP instead of SEXPREC*. So, I don't think we need SEXPREC in theory. But, I might be wrong, and I don't have a strong preference on this.

yutannihilation avatar Nov 24 '23 01:11 yutannihilation

The winapi crate is an example of a major crate offering the pointer alias as well as the pointed-to types as well.

Generally, I would suggest defining every single part as close as possible to the C version, including making SEXP just be a type alias of a pointer.

Lokathor avatar Nov 27 '23 19:11 Lokathor

Just curious. Why do you think it's better? I agree "defining every single part as close as possible to the C version" sounds good as a general rule, but I don't see any actual benefits in this case.

yutannihilation avatar Nov 27 '23 23:11 yutannihilation

Users needing example code, and/or needing to talk to others about the lib (usage, bug reports, etc), will generally need to be able to talk about it in terms of C code. Then they'll have to convert that discussion about C mentally back into Rust code before they can actually write Rust. Having the most minimal difference possible, ideally none at all, reduces the mental load of the whole cycle.

Lokathor avatar Nov 28 '23 00:11 Lokathor

I see. Then, I personally think this crate is not the case as I haven't seen anyone talk about SEXPREC whether it's C code or Rust code. But, it might be just my preference. Thanks for the explanation.

yutannihilation avatar Nov 28 '23 00:11 yutannihilation