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.

Sign up for GitHub

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

Jump to bottom

Use traits to define common backend interface #196

Merged
merged 5 commits into from
Feb 14, 2024
Merged

Use traits to define common backend interface #196

merged 5 commits into from
Feb 14, 2024

Conversation

ids1024
Copy link
Member

@ids1024 ids1024 commented Jan 27, 2024

This still uses enums to dispatch to the backends, but has the *Dispatch types implement the same traits. Nothing about the behavior or performance should change.

If we require backends to implement the same methods, it seems good to use traits for that. Any documentation on the interface implemented by backends can be in one place. The traits can provide default implementations where appropriate.

Hopefully this will help with more refactoring and features.

@ids1024
Copy link
Member Author

ids1024 commented Jan 27, 2024

I see some tests are failing since an indirect dev-dependency requires Rust 1.70... not sure what we want to do there.

Though that's unrelated to this change.

notgull
notgull requested changes Jan 27, 2024
View reviewed changes
Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

We should move the backends into a separate directory now, since there are other non-backend files in the root of the crate.

Aside from that, LGTM.

daxpedda
daxpedda approved these changes Jan 28, 2024
View reviewed changes
Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

LGTM!

Would love to get rid of the macro, but didn't look into it to offer any suggestions.
I second #196 (review) suggestion, but I'm happy to approve it as is.

@ids1024
Copy link
Member Author

ids1024 commented Jan 28, 2024

The alternative to the macro would be Box<dyn ...>, but that's probably not possible anyway with the associated type.

I think the enum_dispatch crate would be a popular solution here, but that adds a proc macro dependency.

@notgull
Copy link
Member

notgull commented Jan 28, 2024

I see some tests are failing since an indirect dev-dependency requires Rust 1.70... not sure what we want to do there.

Rebase on master for fixed MSRV CI

@ids1024
Use traits to define common backend interface
7f95385 
This still uses enums to dispatch to the backends, but has the `*Dispatch`
types implement the same traits. Nothing about the behavior or
performance should change.

If we require backends to implement the same methods, it seems good to
use traits for that. Any documentation on the interface implemented by
backends can be in one place. The traits can provide default
implementations where appropriate.

Hopefully this will help with more refactoring and features.
notgull
notgull requested changes Feb 13, 2024
View reviewed changes
Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

I maintain my earlier comment about the separate directory. I would move cg.rs, kms.rs, etc into a separate sys module to separate them from the new ones.

@ids1024
ci: Downgrade dep for 1.65.0 build
8286a5b
@ids1024
Add #[allow(dead_code)] to SwResultExt
878d51c 
Fixes build warnings on some platforms.
@ids1024
Move backends into a backends module
c0ff669
@ids1024
Move new() methods into traits
d7b32fa 
Now `lib.rs` doesn't need conditional code and macros for dispatching
`new`. It can be handled by `make_dispatch!`.
@ids1024
Copy link
Member Author

ids1024 commented Feb 14, 2024

Added fixes for (unrelated) CI issues, and moved the backends to a backends module.

@ids1024
Copy link
Member Author

ids1024 commented Feb 14, 2024

Added a commit that move the backend context/surface ::new methods into traits, making it possible for make_dispatch! to handle new like it handles other methods.

@ids1024 ids1024 requested a review from notgull February 14, 2024 05:20
notgull
notgull approved these changes Feb 14, 2024
View reviewed changes
Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@ids1024 ids1024 merged commit 6a45203 into master Feb 14, 2024
39 checks passed
@ids1024 ids1024 deleted the traits branch February 14, 2024 16:12
@notgull notgull mentioned this pull request Apr 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@notgull notgull notgull approved these changes

@daxpedda daxpedda daxpedda approved these changes

@jackpot51 jackpot51 Awaiting requested review from jackpot51 jackpot51 is a code owner

@madsmtm madsmtm Awaiting requested review from madsmtm madsmtm is a code owner

@john01dav john01dav Awaiting requested review from john01dav

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ids1024 @notgull @daxpedda