Skip to content
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

Make testing equivalence of floats in SITL a compilation error #9196

Merged
merged 6 commits into from
Aug 27, 2018

Conversation

peterbarker
Copy link
Contributor

No description provided.

@peterbarker
Copy link
Contributor Author

The last patch here has me a little dubious; should we really be using an epsilon?

Perhaps we should instead pragma-off the compiler error for test_math.cpp?

tridge
tridge previously requested changes Aug 16, 2018
Copy link
Contributor

@tridge tridge left a comment

Choose a reason for hiding this comment

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

this will break xplane

@@ -241,7 +241,7 @@ bool XPlane::receive_data(void)
*/
bool has_magic = ((uint32_t)(data[1] * throttle_magic_scale) % 1000U) == (uint32_t)(throttle_magic * throttle_magic_scale);
if (data[1] < 0 ||
data[1] == throttle_sent ||
is_equal(data[1], throttle_sent) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

no, this really, really needs to be ==, a epsilon will break this

@peterbarker
Copy link
Contributor Author

Assigned devcalltopic in case somebody on the call knows the tests should be strict-equivalence.

@peterbarker
Copy link
Contributor Author

It was decided on the devcall that the math tests should be allowed to directly compare floats for equivalence, on the basis that they should know what they're doing.

Removed old changes, pragma'd away the -Wfloat-equal.

@peterbarker peterbarker merged commit 43f3d61 into ArduPilot:master Aug 27, 2018
@peterbarker peterbarker deleted the sitl-float-equal branch August 27, 2018 23:58
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.

None yet

3 participants