-
Notifications
You must be signed in to change notification settings - Fork 745
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
Add support for visiting floating point values #1507
Changes from 4 commits
f1774e4
373a77f
0724604
7c6b83b
8ded0f1
b144620
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,14 +36,77 @@ pub(crate) struct MatchVisitor<'a> { | |
inner: &'a SpanMatch, | ||
} | ||
|
||
#[derive(Debug, Clone, PartialOrd, Ord, Eq, PartialEq)] | ||
#[derive(Debug, Clone)] | ||
pub(crate) enum ValueMatch { | ||
Bool(bool), | ||
F64(f64), | ||
U64(u64), | ||
I64(i64), | ||
NaN, | ||
Pat(Box<MatchPattern>), | ||
} | ||
|
||
impl Eq for ValueMatch {} | ||
|
||
impl PartialEq for ValueMatch { | ||
fn eq(&self, other: &Self) -> bool { | ||
use ValueMatch::*; | ||
match (self, other) { | ||
(Bool(a), Bool(b)) => a.eq(b), | ||
(F64(a), F64(b)) => { | ||
debug_assert!(!a.is_nan()); | ||
debug_assert!(!b.is_nan()); | ||
|
||
a.eq(b) | ||
} | ||
(U64(a), U64(b)) => a.eq(b), | ||
(I64(a), I64(b)) => a.eq(b), | ||
(NaN, NaN) => true, | ||
(Pat(a), Pat(b)) => a.eq(b), | ||
_ => false, | ||
} | ||
} | ||
} | ||
|
||
impl Ord for ValueMatch { | ||
fn cmp(&self, other: &Self) -> Ordering { | ||
use ValueMatch::*; | ||
match (self, other) { | ||
(Bool(this), Bool(that)) => this.cmp(that), | ||
(Bool(_), _) => Ordering::Less, | ||
|
||
(F64(this), F64(that)) => this | ||
.partial_cmp(that) | ||
.expect("`ValueMatch::F64` may not contain `NaN` values"), | ||
(F64(_), Bool(_)) => Ordering::Greater, | ||
(F64(_), _) => Ordering::Less, | ||
|
||
(NaN, NaN) => Ordering::Equal, | ||
(NaN, Bool(_)) | (NaN, F64(_)) => Ordering::Greater, | ||
(NaN, _) => Ordering::Less, | ||
|
||
(U64(this), U64(that)) => this.cmp(that), | ||
(U64(_), Bool(_)) | (U64(_), F64(_)) | (U64(_), NaN) => Ordering::Greater, | ||
(U64(_), _) => Ordering::Less, | ||
|
||
(I64(this), I64(that)) => this.cmp(that), | ||
(I64(_), Bool(_)) | (I64(_), F64(_)) | (I64(_), NaN) | (I64(_), U64(_)) => { | ||
Ordering::Greater | ||
} | ||
(I64(_), _) => Ordering::Less, | ||
|
||
(Pat(this), Pat(that)) => this.cmp(that), | ||
(Pat(_), _) => Ordering::Greater, | ||
} | ||
} | ||
} | ||
|
||
impl PartialOrd for ValueMatch { | ||
fn partial_cmp(&self, other: &Self) -> Option<Ordering> { | ||
Some(self.cmp(other)) | ||
} | ||
} | ||
|
||
#[derive(Debug, Clone)] | ||
pub(crate) struct MatchPattern { | ||
pub(crate) matcher: Pattern, | ||
|
@@ -127,13 +190,22 @@ impl PartialOrd for Match { | |
|
||
// === impl ValueMatch === | ||
|
||
fn value_match_f64(v: f64) -> ValueMatch { | ||
if v.is_nan() { | ||
ValueMatch::NaN | ||
} else { | ||
ValueMatch::F64(v) | ||
} | ||
} | ||
|
||
impl FromStr for ValueMatch { | ||
type Err = matchers::Error; | ||
fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
s.parse::<bool>() | ||
.map(ValueMatch::Bool) | ||
.or_else(|_| s.parse::<u64>().map(ValueMatch::U64)) | ||
.or_else(|_| s.parse::<i64>().map(ValueMatch::I64)) | ||
.or_else(|_| s.parse::<f64>().map(value_match_f64)) | ||
.or_else(|_| { | ||
s.parse::<MatchPattern>() | ||
.map(|p| ValueMatch::Pat(Box::new(p))) | ||
|
@@ -145,6 +217,8 @@ impl fmt::Display for ValueMatch { | |
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
match self { | ||
ValueMatch::Bool(ref inner) => fmt::Display::fmt(inner, f), | ||
ValueMatch::F64(ref inner) => fmt::Display::fmt(inner, f), | ||
ValueMatch::NaN => fmt::Display::fmt(&f64::NAN, f), | ||
ValueMatch::I64(ref inner) => fmt::Display::fmt(inner, f), | ||
ValueMatch::U64(ref inner) => fmt::Display::fmt(inner, f), | ||
ValueMatch::Pat(ref inner) => fmt::Display::fmt(inner, f), | ||
|
@@ -275,6 +349,18 @@ impl SpanMatch { | |
} | ||
|
||
impl<'a> Visit for MatchVisitor<'a> { | ||
fn record_f64(&mut self, field: &Field, value: f64) { | ||
match self.inner.fields.get(field) { | ||
Some((ValueMatch::NaN, ref matched)) if value.is_nan() => { | ||
matched.store(true, Release); | ||
} | ||
Some((ValueMatch::F64(ref e), ref matched)) if value == *e => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably need special behavior for I would probably change this by adding a separate Some((ValueMatch::NaN, ref matched)) if value.is_nan() => {
matched.store(true, Release);
} This way, users can match any This also avoids the problem of NaNs in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @hawkw I had a question for this particular code location, unrelated to the NaN support. Should this method here perform an equality compare against the matching value, or should it do an epsilon compare? Clippy warns against doing an equality compare but I'm not sure it feels right to be doing an epsilon compare here. Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's probably best to do an epsilon compare here. In most cases, I think the user will be looking for values that are logically "equal" to the provided decimal number, rather than looking for floating-point values whose bit patterns are exactly equal. Either way, it's probably worth documenting how floats are compared in the documentation for |
||
matched.store(true, Release); | ||
} | ||
_ => {} | ||
} | ||
} | ||
|
||
fn record_i64(&mut self, field: &Field, value: i64) { | ||
use std::convert::TryInto; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,8 +19,9 @@ pub struct MockField { | |
value: MockValue, | ||
} | ||
|
||
#[derive(Debug, Eq, PartialEq)] | ||
#[derive(Debug)] | ||
pub enum MockValue { | ||
F64(f64), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor nit, but since the test support code now handles There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where would be the best place to put these? I looked for example in the tracing-core/src/field.rs but most of the tests there were calling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
I64(i64), | ||
U64(u64), | ||
Bool(bool), | ||
|
@@ -29,6 +30,31 @@ pub enum MockValue { | |
Any, | ||
} | ||
|
||
impl Eq for MockValue {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is only correct if we ensure no There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
|
||
impl PartialEq for MockValue { | ||
fn eq(&self, other: &Self) -> bool { | ||
use MockValue::*; | ||
|
||
match (self, other) { | ||
(F64(a), F64(b)) => { | ||
debug_assert!(!a.is_nan()); | ||
debug_assert!(!b.is_nan()); | ||
|
||
a.eq(b) | ||
} | ||
(I64(a), I64(b)) => a.eq(b), | ||
(U64(a), U64(b)) => a.eq(b), | ||
(Bool(a), Bool(b)) => a.eq(b), | ||
(Str(a), Str(b)) => a.eq(b), | ||
(Debug(a), Debug(b)) => a.eq(b), | ||
(Any, _) => true, | ||
(_, Any) => true, | ||
_ => false, | ||
} | ||
} | ||
} | ||
|
||
pub fn mock<K>(name: K) -> MockField | ||
where | ||
String: From<K>, | ||
|
@@ -120,6 +146,7 @@ impl Expect { | |
impl fmt::Display for MockValue { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
match self { | ||
MockValue::F64(v) => write!(f, "f64 = {:?}", v), | ||
MockValue::I64(v) => write!(f, "i64 = {:?}", v), | ||
MockValue::U64(v) => write!(f, "u64 = {:?}", v), | ||
MockValue::Bool(v) => write!(f, "bool = {:?}", v), | ||
|
@@ -136,6 +163,11 @@ pub struct CheckVisitor<'a> { | |
} | ||
|
||
impl<'a> Visit for CheckVisitor<'a> { | ||
fn record_f64(&mut self, field: &Field, value: f64) { | ||
self.expect | ||
.compare_or_panic(field.name(), &value, &self.ctx[..]) | ||
} | ||
|
||
fn record_i64(&mut self, field: &Field, value: i64) { | ||
self.expect | ||
.compare_or_panic(field.name(), &value, &self.ctx[..]) | ||
|
@@ -180,6 +212,10 @@ impl<'a> From<&'a dyn Value> for MockValue { | |
} | ||
|
||
impl Visit for MockValueBuilder { | ||
fn record_f64(&mut self, _: &Field, value: f64) { | ||
self.value = Some(MockValue::F64(value)); | ||
} | ||
|
||
fn record_i64(&mut self, _: &Field, value: i64) { | ||
self.value = Some(MockValue::I64(value)); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly to
Ord
, theEq
implementation is only correct if we ensure there are noNaN
values present in theValueMatch::F64
variant. See my comment on theOrd
impl for details.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍