Skip to content

Make the digital::{v1, v2}::InputPin traits proven #164

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
Merged
merged 3 commits into from
Nov 17, 2019
Merged

Make the digital::{v1, v2}::InputPin traits proven #164

merged 3 commits into from
Nov 17, 2019

Conversation

nils-van-zuijlen
Copy link
Contributor

See #41

Previous attempt : #102

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @thejpster (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

therealprof
therealprof requested changes Nov 15, 2019
View reviewed changes
Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

I'm okay with removing the unproven feature from digital::v2 but we absolutely don't want a breaking change to put something proven behind a feature gate.

@nils-van-zuijlen
Copy link
Contributor Author

@therealprof I don't understand what you said. Do you mean that the v1 trait should be proven also?

I did not remove the unproven feature flag there because to me it does not make sense to prove a deprecated trait.

And keeping the v1 trait behind a feature flag forces the compat traits to be behind it.

therealprof
therealprof reviewed Nov 15, 2019
View reviewed changes
Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

It's a compatibility hazard to add feature gates to code which was previously unguarded. I'm fine with any change which doesn't break compatbility with existing applications.

src/digital/v2_compat.rs Outdated
@@ -115,6 +117,7 @@ mod tests {
}
}

#[cfg(feature = "unproven")]
Copy link
Contributor

Choose a reason for hiding this comment

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

You added this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

v1::StatefulOutputPin is unproven, so this test wouldn't build without the flag anyway.

src/digital/v2_compat.rs Outdated
@@ -140,10 +144,12 @@ mod tests {
}
}

#[cfg(feature = "unproven")]
Copy link
Contributor

Choose a reason for hiding this comment

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

... and this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

v2::ToggleableOutputPin is unproven, so this test wouldn't build without the flag anyway.

src/digital/v2_compat.rs Outdated
struct NewToggleablePinConsumer<T: v2::ToggleableOutputPin> {
_pin: T,
}

#[cfg(feature = "unproven")]
Copy link
Contributor

Choose a reason for hiding this comment

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

... and this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

v2::ToggleableOutputPin is unproven, and NewToggleablePinConsumer depends on it, so this test wouldn't build without the flag anyway.

src/digital/v2_compat.rs Outdated
@@ -153,6 +159,7 @@ mod tests {
}
}

#[cfg(feature = "unproven")]
Copy link
Contributor

Choose a reason for hiding this comment

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

... and this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as #164 (comment)

@nils-van-zuijlen
Copy link
Contributor Author

I added these unproven feature flags to make tests work even when that flag is not enabled.

@nils-van-zuijlen nils-van-zuijlen changed the title Makes the digital::v2::InputPin trait proven Make the digital::{v1, v2}::InputPin traits proven Nov 17, 2019
@burrbull
Copy link
Member

I added these unproven feature flags to make tests work even when that flag is not enabled.

Remove them.

@nils-van-zuijlen
Makes the digital::v2::InputPin trait proven
1dad061 
See rust-embedded#41 for more details about this trait.

Removes the feature flag from v2 implementation

Keeps it on v1 because it would not make sense to prove a deprecated
feature.
@nils-van-zuijlen nils-van-zuijlen force-pushed the prove-InputPin branch 2 times, most recently from 26b5b18 to bea9bb6 Compare November 17, 2019 19:47
@nils-van-zuijlen
Make the digital::v1::InputPin proven
bea9bb6 
Side effect: make the compat traits proven
therealprof
therealprof requested changes Nov 17, 2019
View reviewed changes
Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

Please add a CHANGELOG.md entry, then this should be ready to go.

therealprof
therealprof approved these changes Nov 17, 2019
View reviewed changes
Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@therealprof
Copy link
Contributor

bors r+

bors bot added a commit that referenced this pull request Nov 17, 2019
@bors @nils-van-zuijlen
Merge #164
db09d73 
164: Make the digital::{v1, v2}::InputPin traits proven r=therealprof a=nils-van-zuijlen

See #41 

Previous attempt : #102 

Co-authored-by: Nils VAN ZUIJLEN <[email protected]>
@bors
Copy link
Contributor

bors bot commented Nov 17, 2019

Build succeeded

@bors bors bot merged commit d9d1052 into rust-embedded:master Nov 17, 2019
@nils-van-zuijlen nils-van-zuijlen deleted the prove-InputPin branch November 17, 2019 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@therealprof therealprof therealprof approved these changes

@ilya-epifanov ilya-epifanov Awaiting requested review from ilya-epifanov

@thejpster thejpster Awaiting requested review from thejpster

Assignees

@thejpster thejpster

Labels
S-waiting-on-review Review is incomplete T-hal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nils-van-zuijlen @rust-highfive @burrbull @therealprof @thejpster