8000 libraries/tock-cells/OptionalCell: remove `T: Default` constraint by lschuermann · Pull Request #3334 · tock/tock · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

libraries/tock-cells/OptionalCell: remove T: Default constraint #3334

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 1 commit into from
Nov 28, 2022
Merged

libraries/tock-cells/OptionalCell: remove T: Default constraint #3334

merged 1 commit into from
Nov 28, 2022

Conversation

lschuermann
Copy link
Member

Pull Request Overview

#[derive(Default)] incorrectly constraints T: Default, whereas <Option<T> as Default>::default() will return None. By manually implementing the trait we can circumvent this restriction.

Testing Strategy

This pull request was tested by compiling.

TODO or Help Wanted

N/A

Documentation Updated

  • Updated the relevant files in /docs, or no updates are required.

Formatting

  • Ran make prepush.

@lschuermann lschuermann added the P-Upkeep This a relatively minor change, or one that is limited in scope, and requires less scrutiny. label Nov 26, 2022
@github-actions github-actions bot added the tock-libraries This affects libraries supported by the Tock project label Nov 26, 2022
@lschuermann
libraries/tock-cells/OptionalCell: remove T: Default constraint
c92c25c 
`#[derive(Default)]` incorrectly constraints T: Default, whereas
`<Option<T> as Default>::default()` will return None. By manually
implementing the trait we can circumvent this restriction.

Signed-off-by: Leon Schuermann <leon@is.currently.online>
@lschuermann lschuermann force-pushed the dev/optionalcell-default branch from 1e61b18 to c92c25c Compare November 26, 2022 23:27
@lschuermann lschuermann changed the title libraries/tock-cells/optional_cell: remove T: Default constraint libraries/tock-cells/OptionalCell: remove T: Default constraint Nov 26, 2022
jrvanwhy
jrvanwhy approved these changes Nov 28, 2022
View reviewed changes
Copy link
Contributor
@jrvanwhy jrvanwhy left a comment

Choose a reason for hiding this comment

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

empty and default seem kinda redundant now.

@bradjc
Copy link
Contributor
bradjc commented Nov 28, 2022

Do we need to implement default at all?

@hudson-ayers
Copy link
Contributor

Leon needs it for https://github.com/tock/tock/pull/3110/files#diff-a490ef2e2704690868ab20d6748f21da7749a0a8dc8b16e95825cf8dd861760dR67 I believe

@lschuermann
Copy link
Member Author
lschuermann commented Nov 28, 2022 •
edited
Loading

Leon needs it for https://github.com/tock/tock/pull/3110/files#diff-a490ef2e2704690868ab20d6748f21da7749a0a8dc8b16e95825cf8dd861760dR67 I believe

That's right. The perhaps most useful property of the Default trait is its ability to (recursively) initialize more complex types which otherwise we wouldn't be able to initialize cheaply without usage of unsafe, e.g. arrays of elements implementing Default. Option<T> is a useful gadget to build a wrapper type implementing Default around a non-default T. However, the restriction for T: Default on OptionalCell makes it unusable for these applications.

@lschuermann lschuermann mentioned this pull request Nov 28, 2022
Merged
8 tasks
@hudson-ayers
Copy link
Contributor

empty and default seem kinda redundant now.

I think that is fine, empty is used in a bunch of code including out-of-tree code and I don't think the redundancy is worth breaking that code, plus empty has somewhat more obvious behavior than default anyway.

bors r+

@jrvanwhy
Copy link
Contributor

I just noticed that empty is const, so they're not actually redundant.

@bors
Copy link
Contributor
bors bot commented Nov 28, 2022

Build succeeded:

@bors bors bot merged commit ae539b6 into tock:master Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alistair23 alistair23 alistair23 approved these changes

@bradjc bradjc bradjc approved these changes

@jrvanwhy jrvanwhy jrvanwhy approved these changes

@hudson-ayers hudson-ayers hudson-ayers approved these changes

@twilfredo twilfredo twilfredo approved these changes

Assignees
No one assigned
Labels
P-Upkeep This a relatively minor change, or one that is limited in scope, and requires less scrutiny. tock-libraries This affects libraries supported by the Tock project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lschuermann @bradjc @hudson-ayers @jrvanwhy @alistair23 @twilfredo
0