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

Use const instead of macros #5

Merged
merged 17 commits into from
Jan 31, 2023

Conversation

sosthene-nitrokey
Copy link
Contributor

@sosthene-nitrokey sosthene-nitrokey commented Aug 22, 2022

This is pretty much a complete rewrite. It gives a much nicer API to
understand from the documentation. It also means that it doesn't rely on
#[doc(hidden)] methods (except for Interchange::const_new so that it
can be removed once the required compiler features are stabilized)

Because everything doesn't need to be 'static anymore, it can also be
tested efficiently with Loom, ensuring that there are no possible race
conditions.

It is a significantly breaking change

There may be some possible improvement. For example the claims mechanism could be implemented in the Channel struct and use RAII to be made safe and remove the need for reset-claims. This could make the API 100% safe which would be nice, but it would not be zero cost, given that it's unlikely a Requester or a Responder would ever be dropped in most use cases.

@sosthene-nitrokey sosthene-nitrokey force-pushed the const-interchange branch 2 times, most recently from d685227 to da59528 Compare August 23, 2022 16:45
@sosthene-nitrokey
Copy link
Contributor Author

sosthene-nitrokey commented Aug 25, 2022

I've updated the PR with this trick which makes it possible to have a const constructor and completely remove all macros.

I also added an RAII mechanism instead of the reset_claims which should not be that big of an overhead. It also makes Channel::split safe which means that no public API is unsafe, and that Channel can be used by itself without using the Interchange structure.

This is pretty much a complete rewrite. It gives a much nicer API to
understand from the documentation. It also means that it doesn't rely on
\#[doc(hidden)] methods (except for `Interchange::const_new` so that it
can be removed once the required compiler features are stabilized)

Because everything doesn't need to be `'static` anymore, it can also be
tested efficiently with Loom, ensuring that there are no possible race
conditions.

It is a significantly breaking change
This method doesn't make sense now with RAII
@sosthene-nitrokey
Copy link
Contributor Author

I've made a branch that also removes the Clone trait bound on the request and response structs. sosthene-nitrokey#1

@robin-nitrokey
Copy link
Member

It would be helpful to also implement Default for Interchange and Channel.

@robin-nitrokey
Copy link
Member

robin-nitrokey commented Jan 16, 2023

Ah, sorry, I probably looked at the wrong branch.

src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@nickray
Copy link
Member

nickray commented Jan 16, 2023

Not worried about the overhead of the atomic bools to implement the RAII, just to state the obvious though, it's not guaranteed destructors (drop) are run. Seems fine for the intended use cases.

Overall, I think these are good changes, particularly adding the testing with Loom. Do we have any chance of upstreaming constness as needed to Loom, or is this a property of the implementations there?

Also.. did Loom catch any bugs already?

src/lib.rs Outdated
/// (rq, rp) = interchange.claim().unwrap() ;
/// }
/// ```
pub struct Interchange<Q, A, const N: usize> {
Copy link
Member

@nickray nickray Jan 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presume Q = query/question, and A = answer? Maybe use longer names in the struct definition (and switch to the short ones in the implementation)? In any case, please add a comment on what the names mean, so it's easier to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I'm not very satisfied with Q,A since everything else mentions Request/Response. I'll replace it with Rq and Rp

@sosthene-nitrokey
Copy link
Contributor Author

This is the implementation of new for loom's atomic mocks.

It registers itself to loom's runtime so I doubt it will ever be made const.
It may be possible to make it const by making the registration happen lazily on the first operation, but I don't know if it's worth it or if it can work.

@sosthene-nitrokey
Copy link
Contributor Author

I also added an InterchangeRef type that removes the need to have a const parameter everywhere.

static INTERCHANGE_INNER: Interchange<Request, Response, 1> = Interchange::new();

// The size of the interchange is absent from the type
static INTERCHANGE: InterchangeRef<'static, Request, Response> = INTERCHANGE_INNER.as_interchange_ref();

let (mut rq, mut rp) = INTERCHANGE.claim().unwrap();

@robin-nitrokey robin-nitrokey merged commit a4532e3 into trussed-dev:main Jan 31, 2023
sosthene-nitrokey added a commit to sosthene-nitrokey/interchange that referenced this pull request Mar 31, 2023
PR trussed-dev#5 ended up adding an RAII mechanism that mad the split API safe
nickray pushed a commit that referenced this pull request Apr 6, 2023
PR #5 ended up adding an RAII mechanism that mad the split API safe
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.

3 participants