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

A class to represent an approximate quantity #2285

Merged
merged 8 commits into from
Aug 17, 2019

Conversation

pleroy
Copy link
Member

@pleroy pleroy commented Aug 15, 2019

First part of addressing #2269.

pleroy added 6 commits August 14, 2019 21:17

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
testing_utilities/approximate_quantity_body.hpp Outdated Show resolved Hide resolved
testing_utilities/approximate_quantity_body.hpp Outdated Show resolved Hide resolved
testing_utilities/approximate_quantity_body.hpp Outdated Show resolved Hide resolved
error_representation.size() >= 2 &&
error_representation[0] == '0' &&
(error_representation[1] == 'x' || error_representation[1] == 'X');
for (int i = 0; i < error_representation.size(); ++i) {
Copy link
Member

Choose a reason for hiding this comment

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

I think that this would be more readable if we were building error_representation (by looping on representation and appending) than rewriting it with 0s; either way, add a comment describing what we are doing.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would not be more readable because I would need more state variables to copy the exponent without patching it. Added a comment.

Sorry, something went wrong.

return ApproximateQuantity<double>::Parse(representation, /*ulp=*/8);
}

ApproximateQuantity<double> operator""_⑼(char const* const representation) {
Copy link
Member

Choose a reason for hiding this comment

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

If we are going to do this beyond ⑴, add 🄐 .. 🄕 as well (morally they should CHECK for a hexadecimal representation but I am OK with trusting the sanity of the author here).

Maybe invest in a macro to factor the definitions?

Historical note: the parenthesized letters were encoded for compatibility with an ARIB standard https://www.unicode.org/wg2/docs/n3469.pdf (they are ARIB 9433..9458); some of the parenthesized digits are also present in that standard (ARIB 9417..9428), though they stop at 12 in ARIB (the parenthesized digits are from Unicode 1.1, so the rationale for their encoding may be lost to time).

Sorry, something went wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, with check and macro.

testing_utilities/approximate_quantity_test.cpp Outdated Show resolved Hide resolved
@eggrobin eggrobin added the LGTM label Aug 16, 2019
@pleroy pleroy merged commit c86fab2 into mockingbirdnest:master Aug 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants