Skip to content

Conversation

@madmongo1
Copy link
Collaborator

error was in finish() in exp3 state

return tab[exp];
if (exp < -308 || exp > 308)
{
return std::pow(10.0, exp);
Copy link
Member

Choose a reason for hiding this comment

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

idk about this

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll get to it. I wanted to prevent the assert from happening

Copy link
Member

@vinniefalco vinniefalco Jan 13, 2020

Choose a reason for hiding this comment

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

Do we still need this check? We need to add tests for the exponent edge cases

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we could just check for "more than 3 digits" and then rely on the finalize to detect the over/underflow. I think i'd prefer that...

oh wait. What if the input is 0.[ten thousand zeroes]1e10000 ?
there's no reason we shouldn't parse this correctly.

It would be trivial to compute the exponent limits on entry into this state

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my view the check is superfluous. it's more expensive to perform the check than to fail late.

Copy link
Member

Choose a reason for hiding this comment

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

In general I think it is good to fail as soon as we know we have an invalid input. So if we get 309 as the exponent, we can fail right there. I'm not happy about having a potential call to std::pow, especially when we know that the exponent is out of range.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

0.0000000001e309 is actually 1e289. stod will parse this correctly.

@vinniefalco
Copy link
Member

The first line of a commit message should be short, certainly not with a number having 500 zeroes...

There's no need to quote the number in the commit message, since it is already visible in the test.

@vinniefalco
Copy link
Member

This should be squashed down into just one commit

Fixes a number of issues. Number parsing tests now pass, including edge cases.
Remaining: examine options for str to double rounding corrections when
number is outside the range 1e-22->1e22
ref: David Gray's seminal work https://ampl.com/netlib/fp/dtoa.c
@vinniefalco
Copy link
Member

I think we should merge the mant and mantn states as you proposed today

// abs(mantissa) < 1
auto start = dig_ - sig_;
auto exponent_adjust = -start - 1;
exp_ += exponent_adjust;
Copy link
Member

@vinniefalco vinniefalco Jan 14, 2020

Choose a reason for hiding this comment

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

despite all the naming fanfare it produces warnings:

warning C4244: '+=': conversion from 'int' to 'short', possible loss of data

Copy link
Member

@vinniefalco vinniefalco Jan 14, 2020

Choose a reason for hiding this comment

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

When you take away the unnecessary local variables and simplify the formulas, the interesting relationship between the values becomes visible:

        if (pos_ == 0)
        {
            // abs(mantissa) < 1
            exp_ = static_cast<short>(
                exp_ + sig_ - dig_ - 1);
        }
        else
        {
            // abs(mantissa) >= 1
            exp_ = static_cast<short>(
                exp_ + pos_ - sig_);
        }

Copy link
Contributor

@ofenloch ofenloch Dec 21, 2022

Choose a reason for hiding this comment

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

I don't know if my problem is related to your discussion. I have a simple Catch2 test for boost.json 1.75.0 that fails because float/double values are not written/read properly:

TEST_CASE("boost::json")
{
    // There seems to be an issue with number formatting.

    // create a boost::json::value
    boost::json::value jv_original = {
        {"pi", 3.14159265359},
        {"e", 2.71828182846}};

    // serialize the boost::json::value to a string
    std::string s{boost::json::serialize(jv_original)};
    // and write the string to a file
    std::string file_name = std::tmpnam(nullptr);
    file_name += ".json";
    std::ofstream ofs(file_name.c_str());
    ofs << s;
    ofs.close();

    // now read the file into a string
    std::ifstream ifs(file_name);
    std::string content((std::istreambuf_iterator<char>(ifs)),
                        (std::istreambuf_iterator<char>()));
    ifs.close();
    std::remove(file_name);

    // and create another boost::json::value from that string
    boost::system::error_code parseErr;
    boost::json::value jv_unserialized = boost::json::parse(content, parseErr);

    // compare both boost::json::value objects
    CHECK(jv_original == jv_unserialized);

} // TEST_CASE("boost::json")

The error message from Catch2 is

[ctest]   CHECK( jv_original == jv_unserialized )
[ctest] with expansion:
[ctest]   {"pi":3.14159265359E0,"e":2.71828182846E0}
[ctest]   ==
[ctest]   {"pi":3.1415926535899996E0,"e":2.71828182846E0}

Copy link
Member

Choose a reason for hiding this comment

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

Please test against recent Boost version. I can't reprodcue your issue locally on 1.81.0

Copy link
Contributor

@ofenloch ofenloch Dec 22, 2022

Choose a reason for hiding this comment

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

Thanks for your answer, @grisumbras.

You're right. I built and installed Boost 1.81.0 (still running on Ubuntu 20.04) and the failure is gone.

Ignore the things I wrote below. I used the pretty_print code from the examples to serialize my object. When I use boost::json::value::serialize everything's fine.

Thanks for your input.

However, I use Boost.JSON 1.81.0 in one of my classes and still have the same problem with this value in a JSON file

"latitude": 79.824228,

Putting this value in the code above results in success. But my compare(lhs, rhs) method says:

[ctest] values of "latitude" differ:
[ctest]   lhs 7.9824228E1
[ctest]   rhs 7.98242E1

I'll investigate further ...

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.

4 participants