Skip to content

Switch from stateless to stateful Clock #27

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

Merged
merged 1 commit into from
Jul 1, 2020
Merged

Conversation

PTaylor-us
Copy link
Member

@PTaylor-us PTaylor-us commented Jun 30, 2020

Currently, the Clock trait is composed of only associated functions. Per #21 (comment), they need to be changed to methods. Specifically, now() needs to take &mut self.

@PTaylor-us
Copy link
Member Author

What happens when more than one instance use the same hardware? Perhaps this can be prevented by consuming the resource upon instantiation.

@PTaylor-us PTaylor-us changed the base branch from master to v0.6 June 30, 2020 23:41
@PTaylor-us PTaylor-us mentioned this pull request Jun 30, 2020
@PTaylor-us PTaylor-us marked this pull request as ready for review June 30, 2020 23:46
@PTaylor-us PTaylor-us mentioned this pull request Jun 30, 2020
@eldruin
Copy link
Contributor

eldruin commented Jul 1, 2020

Looks good to me so far. You would not be able to have more than one mutable reference at a time to a Clock implementer so I think preventing multiple instances is not an issue.
The problem with consuming valuable resources is that you need to handle how to return them again. Specially when adding fallible interactions this can become a bit messy. c.f. 2. at #21 (comment)

@PTaylor-us
Copy link
Member Author

Looks good to me so far. You would not be able to have more than one mutable reference at a time to a Clock implementer so I think preventing multiple instances is not an issue.
The problem with consuming valuable resources is that you need to handle how to return them again. Specially when adding fallible interactions this can become a bit messy. c.f. 2. at #21 (comment)

Correct me if I'm wrong, but I believe that is entirely an implementer issue and not something the trait needs to handle.

@PTaylor-us
Copy link
Member Author

So I think this is good to merge into the v0.6 branch to be joined shortly with #21 and #29 . Do you agree?

@eldruin
Copy link
Contributor

eldruin commented Jul 1, 2020

The problem with consuming valuable resources is that you need to handle how to return them again. Specially when adding fallible interactions this can become a bit messy. c.f. 2. at #21 (comment)

Correct me if I'm wrong, but I believe that is entirely an implementer issue and not something the trait needs to handle.

That is true. I was only trying to comment on your remark about resource consumption upon instantiation.

So I think this is good to merge into the v0.6 branch to be joined shortly with #21 and #29 . Do you agree?

Yes!

@PTaylor-us PTaylor-us merged commit beb9158 into v0.6 Jul 1, 2020
@PTaylor-us PTaylor-us deleted the clock_objects branch July 1, 2020 19:30
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.

2 participants