Skip to content

Conversation

@danielvallance
Copy link

This PR enables clippy::assertions_on_result_states, because asserting that a Result is_ok(), or is_err() causes the unit test failure messages to be quite unhelpful (as described in issue 337 #337).

It also addresses a couple of other clippy warnings.

Copy link
Collaborator

@XanClic XanClic left a comment

Choose a reason for hiding this comment

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

Looks good, just some nit picks, mainly:

  • If we’re asserting a value in an address that’s been part of the previous code, it should have the same form as previously, i.e. assert on usize::MAX instead of 18446744073709551615 and on 0x600 instead of 1536 if the value was previously given as 0x600. Especially so in examples that end up in documentation.
  • I’m not sure about dropping assert!(foo.is_ok()) from documentation examples just because it’s immediately followed by foo.unwrap(). I’m all for replacing .is_ok() by .unwrap() in tests, but in examples, .is_ok() adds an explicit “We’re not just unwrapping for simplicity’s sake, but this example is actually supposed to show that this operation will return Ok(_)”.

Comment on lines +1690 to +1696
assert_matches!(
slice.subslice(0, 101).unwrap_err(),
Error::OutOfBounds { addr: 101 }
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not up to this PR but a general question to everyone: Why does this (and the ones below) report 101? That address is not part of the requested subslice, instead it should be 100 in my understanding…

I think compute_end_offset() should use addr: cmp::max(self.len(), base) instead of addr: mem_end, and VolatileSlice::offset() should use addr: self.addr + self.size (or something like that) instead of addr: new_addr.

(As for this PR, this is a nice example why using this assertion style is better :))

@danielvallance
Copy link
Author

Hi @XanClic thank you for taking the time to review this, I have addressed your comments :)

Copy link
Collaborator

@XanClic XanClic left a comment

Choose a reason for hiding this comment

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

Thanks! :)

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