Skip to content

test: Fix running spec tests that expect NaN result#484

Merged
gumb0 merged 3 commits intomasterfrom
spectest-nan
Aug 17, 2020
Merged

test: Fix running spec tests that expect NaN result#484
gumb0 merged 3 commits intomasterfrom
spectest-nan

Conversation

@gumb0
Copy link
Collaborator

@gumb0 gumb0 commented Aug 13, 2020

No description provided.

@gumb0 gumb0 force-pushed the spectest-nan branch 4 times, most recently from afa6887 to 9e58854 Compare August 13, 2020 16:01
@codecov
Copy link

codecov bot commented Aug 13, 2020

Codecov Report

Merging #484 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           master     #484    +/-   ##
========================================
  Coverage   99.56%   99.56%            
========================================
  Files          54       54            
  Lines       16858    17015   +157     
========================================
+ Hits        16784    16941   +157     
  Misses         74       74            

@gumb0 gumb0 force-pushed the spectest-nan branch 2 times, most recently from a1c54c0 to 20beae1 Compare August 17, 2020 11:23
@gumb0 gumb0 marked this pull request as ready for review August 17, 2020 11:27
@gumb0 gumb0 requested review from axic and chfast August 17, 2020 11:42
// JSON tests have all values including floats serialized as 64-bit unsigned integers.
const uint64_t uint_value = std::stoull(v.at("value").get<std::string>());
/// The unsigned integer type matching the size of this floating-point type.
using UintType = std::conditional_t<std::is_same_v<T, float>, uint32_t, uint64_t>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
using UintType = std::conditional_t<std::is_same_v<T, float>, uint32_t, uint64_t>;
using UintType = FP<T>::UintType;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Collaborator

@chfast chfast left a comment

Choose a reason for hiding this comment

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

Please squash two last commits.

@gumb0 gumb0 merged commit a121d0f into master Aug 17, 2020
@gumb0 gumb0 deleted the spectest-nan branch August 17, 2020 16:44
assert(!is_canonical_nan(v) && !is_arithmetic_nan(v));

// JSON tests have all values including floats serialized as 64-bit unsigned integers.
const uint64_t uint_value = std::stoull(v.at("value").get<std::string>());
Copy link
Member

Choose a reason for hiding this comment

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

@gumb0 Did not mention before merging, because this question affected master too (e.g. not introduced via this PR): do we want to wrap any of these in proper exception handling for invalid inputs (see https://en.cppreference.com/w/cpp/string/basic_string/stoul for invalid_argument or out_of_range) or it is fine leaving it to the general exception catching we have?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually it would be nice to output a line number in the case of exception, now you see only that exception happened somewhere in the file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait until it happens again.

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