Don't panic on negative epsilon #537
Conversation
urschrei
force-pushed
the
simplification_fix
branch
from
dc6e803
to
d5d7a25
Compare
|
🤦♂️ |
urschrei
force-pushed
the
simplification_fix
branch
from
d5d7a25
to
e2465c7
Compare
| @@ -19,6 +19,10 @@ fn rdp<T>(points: &[Point<T>], epsilon: &T) -> Vec<Point<T>> | |||
| where | |||
| T: Float, | |||
| { | |||
| // Epsilon must be greater than zero for any meaningful simplification to happen | |||
| if *epsilon <= T::zero() { | |||
There was a problem hiding this comment.
This implies some kind of programmer error right? Should we add a debug assertion failure here?
There was a problem hiding this comment.
I'm assuming the fuzzer runs against release builds and thus won't trigger the debug assertion? But I might be wrong...
There was a problem hiding this comment.
cargo-fuzz builds in release mode with debug assertions
i don't know where the default value for debug-assertions gets set in cargo-fuzz, but i verified locally that debug assertions are enabled
There was a problem hiding this comment.
Yeah, I don't feel confident about how to resolve this.
I can imagine some cases where it would be interesting if it were possible to reach an assert via the fuzzer, and others, like here, where it wouldn't be interesting.
We could have the convention that debug_assert is the former while assert is the latter, but... that seems pretty limiting.
We could make our own assert macros, like debug_assert_unless_fuzzing!, but that seems a bit much.
There was a problem hiding this comment.
To restate, I'm really OK with just merging this as is. 👍
There was a problem hiding this comment.
I am feeling very indecisive too.
| @@ -19,6 +19,10 @@ fn rdp<T>(points: &[Point<T>], epsilon: &T) -> Vec<Point<T>> | |||
| where | |||
| T: Float, | |||
| { | |||
| // Epsilon must be greater than zero for any meaningful simplification to happen | |||
| if *epsilon <= T::zero() { | |||
|
bors r+ |
|
Build succeeded: |
Fixes #536