Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace SEXPREC with something truly opaque #212

Merged
merged 7 commits into from
Apr 21, 2024
Merged

Conversation

CGMossa
Copy link
Member

@CGMossa CGMossa commented Nov 18, 2023

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

@CGMossa CGMossa marked this pull request as ready for review November 19, 2023 13:34
@yutannihilation
Copy link
Contributor

My comment stays the same as #205 (comment); I believe there's no need to expose SEXPREC at all, but I'm approving.

@CGMossa
Copy link
Member Author

CGMossa commented Nov 20, 2023

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.

@yutannihilation
Copy link
Contributor

yutannihilation commented Nov 24, 2023

I get you. But, my understanding is that, the comments on rust-lang/rust-bindgen#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.

@Lokathor
Copy link

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.

@yutannihilation
Copy link
Contributor

yutannihilation commented Nov 27, 2023

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.

@Lokathor
Copy link

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.

@yutannihilation
Copy link
Contributor

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.

CGMossa and others added 7 commits April 21, 2024 22:11
Update maintainer guide

hopefully, a GHA commit shouldn't follow

maybe it needs to be an expression?

what's happening...

please xplain.

right sha..

hopefully, a GHA commit shouldn't follow

hopefully, a GHA commit shouldn't follow

no generating of anything
from the book
rust for rustaceans
* Added `[generate bindings]` command for CI

Update maintainer guide

* Update bindings [skip ci]

* Update MAINTAINERS_GUIDE.md

Co-authored-by: Ilia Kosenkov <[email protected]>

---------

Co-authored-by: CGMossa <[email protected]>
Co-authored-by: Ilia Kosenkov <[email protected]>
@CGMossa CGMossa merged commit 6083cae into master Apr 21, 2024
19 of 20 checks passed
@CGMossa CGMossa deleted the sexprec_is_cvoid branch April 21, 2024 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants