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

Optional cell enhancements #1056

Merged
merged 7 commits into from Jul 7, 2018

Conversation

Projects
None yet
4 participants
@ppannuto
Copy link
Member

ppannuto commented Jul 1, 2018

Pull Request Overview

This pull request adds some additional methods to OptionalCells that will be needed.

Testing Strategy

Compiling.

Documentation Updated

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

Formatting

  • Ran make formatall.

ppannuto added some commits Jul 1, 2018

tock-cells: Add `replace` method to `OptionalCell`
Sometimes you have an `Option` that you'd like to put in an `OptionalCell`,
which means you need to either `set` or `clear` to `OptionalCell` depending
on the `Option` you're putting in. This implements that logic in `OptionalCell`.
@phil-levis
Copy link
Collaborator

phil-levis left a comment

Are these all of the methods that are needed? Or are there more? It might be good to just fill in all of the methods that exist in normal Cell types.

Should all of the Cell types have a relatively uniform (and complete) API?

@alevy

This comment has been minimized.

Copy link
Member

alevy commented Jul 2, 2018

Are these all of the methods that are needed? Or are there more?

There are probably more. It wouldn't be bad to have more functionality implemented, except of course that in the case of safety-sensitive code, it's never great to have functionality that's not really used (cause bad bugs are less likely to be caught).

I don't think adding functions has the same kind of affect on backwards stability as changing existing functions, and so don't think we should block on finishing the API.

Should all of the Cell types have a relatively uniform [...] API?

No. Different "cell" types are pretty semantically different, so I don't think it makes a ton of sense to combine their APIs

tock-cells: add remaining `Option` methods
Add the remaining methods from std `Option` that are sensical for
`OptionalCell`.
@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 3, 2018

I think I'm in agreement with @phil-levis on this one. The OptionalCell is really just a convenience type to make it easier to work with Options that happen to be in a Cell. Though the goal then is to mirror the Option API, not the Cell API.

I went through and added the remaining methods from Option that I think make sense for OptionalCell.

@bradjc
Copy link
Contributor

bradjc left a comment

I think that including unwrap is a mistake, as it is a function we don't want used.

Also, is that replace different from the take cell replace? If so, I think we should use the take cell version.

ppannuto added some commits Jul 3, 2018

tock-cells: remove `unwrap` from OptionalCell
Don't want to encourage panic'ing method use.
@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 3, 2018

Good call on both. Updated.

Resolved both comments.

@bradjc
Copy link
Contributor

bradjc left a comment

To match, replace should just take a T, and not an option, correct?

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 3, 2018

I don't think so. A TakeCell holds a T, and OptionalCell holds an Option<T>

tock-cells: match `replace` for optional and take cells
Introduce `insert` for the new semantics of insering an existing
`Option` into an `OptionalCell`.

I now understand what you meant. Fixed up replace to match the TakeCell signature and added insert for the new mechanism

@ppannuto

This comment has been minimized.

Copy link
Member

ppannuto commented Jul 3, 2018

@phil-levis Do you have any remaining issues with this PR? I can start patching up the other OptionalCell PRs once this goes through.

@alevy

alevy approved these changes Jul 4, 2018

@bradjc

bradjc approved these changes Jul 5, 2018

Copy link
Contributor

bradjc left a comment

Since the tock-cells crates is available, I'm sure someone will be very glad this PR exists :)

@bradjc

This comment has been minimized.

Copy link
Contributor

bradjc commented Jul 6, 2018

@ppannuto ppannuto merged commit a498ab4 into master Jul 7, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@ppannuto ppannuto deleted the optional-cell-enhancements branch Jul 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment