Skip to content

Much improved Quirks chapter, some Concepts work#36

Merged
Lokathor merged 33 commits intomasterfrom
lokathor
Dec 21, 2018
Merged

Much improved Quirks chapter, some Concepts work#36
Lokathor merged 33 commits intomasterfrom
lokathor

Conversation

@Lokathor
Copy link
Member

No description provided.

@Lokathor Lokathor requested a review from Ravenslofty December 18, 2018 21:20
Copy link
Member

@Ravenslofty Ravenslofty left a comment

Choose a reason for hiding this comment

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

Looks okay to me.

Some day we can split the software math into its own crate and do quickcheck there, until then the test code must be fully no_std because otherwise the examples won't build (sadly, dev-dependencies applies to both tests and examples)
@Lokathor Lokathor merged commit c666cc1 into master Dec 21, 2018
Copy link
Contributor

@parasyte parasyte left a comment

Choose a reason for hiding this comment

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

So far I just found some typos.

Also, again signed values can be annoying, because if the value _just happens_
to be `i32::MIN` then when you negate it you'll have... _still_ a negative
value. I'm not 100% on this, but I think the correct thing to do at that point
is to give `$t::MIN` as out output num value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
is to give `$t::MIN` as out output num value.
is to give `$t::MIN` as the output num value.

value. I'm not 100% on this, but I think the correct thing to do at that point
is to give `$t::MIN` as out output num value.

Did you get all that? Good, because this is involves casting, we will need to
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Did you get all that? Good, because this is involves casting, we will need to
Did you get all that? Good, because this involves casting, we will need to

@Lokathor
Copy link
Member Author

It seems like I can't accept the suggestions directly since I already merged this PR, but I'll put it into my files and it'll show up in the next PR.

@parasyte
Copy link
Contributor

Yeah, I was a bit late hitting the submit button. There was a lot to read through.

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.

3 participants