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

Real generator broken in 1.76 #688

Open
chambm opened this issue Jun 3, 2021 · 10 comments
Open

Real generator broken in 1.76 #688

chambm opened this issue Jun 3, 2021 · 10 comments

Comments

@chambm
Copy link

chambm commented Jun 3, 2021

#include <iostream>
#include <string>
#include <limits>
#include <algorithm>
#include <boost/spirit/include/karma.hpp>

int main()
{
    std::cout << BOOST_VERSION << std::endl;
    
    double values[] = { 0.9999, 1.9999, 2.9999, 3.9999, 4.9999, 5.9999, 6.9999, 7.9999, 8.9999, 9.9, 9.9999, 10.99, 10.9999, 11.999, 11.9999 };};
    for (auto value : values)
    {
        std::string s;
        std::back_insert_iterator<std::string> sink(s);
        boost::spirit::karma::generate(sink, boost::spirit::karma::real_generator<>(), value);
        std::cout << s << std::endl;
    }
}

At Coliru as of 6/3/2021, it incorrectly generates:

107600
1.0
2.0
3.0
4.0
5.0
6.0
7.0
8.0
9.0
9.9
1.0
10.99
1.0
11.999
1.0

At rextester it correctly generates:

106501
1.0
2.0
3.0
4.0
5.0
6.0
7.0
8.0
9.0
9.9
10.0
10.99
11.0
11.999
12.0

I'm not sure when it broke, and it's not broken for every input value, but for fairly mundane values it's very wrong. It seems to have something to do with the number of decimals places.

@Kojoley
Copy link
Collaborator

Kojoley commented Jun 4, 2021

Caused by #629. @hkaiser any ideas?

chambm added a commit to ProteoWizard/pwiz that referenced this issue Jun 4, 2021
* NB: bundled Boost tarball reverts a change in Spirit.Karma that made it generate incorrect real number strings (boostorg/spirit#688)
@chambm
Copy link
Author

chambm commented Jun 7, 2021

                if (integer_part >= 10.)
                {
                    integer_part /= 10.;
                    ++dim;
                }

Looks very suspect for this bug. :) I can confirm that reverting #629 fixes the issue.

chambm added a commit to ProteoWizard/pwiz that referenced this issue Jun 7, 2021
* updated Boost to 1.76.0
* NB: bundled Boost tarball reverts a change in the official Spirit.Karma that made it generate incorrect real number strings (boostorg/spirit#688)
* NB: after this update, default Boost.Regex linkage is header-only
@fiesh
Copy link

fiesh commented Jun 9, 2021

Oh good, there's already a ticket for this. This hit us pretty hard in production code.

@hkaiser
Copy link
Collaborator

hkaiser commented Jun 9, 2021

Looks very suspect for this bug. :) I can confirm that reverting #629 fixes the issue.

We'll have to find another fix for #628, then :/

@RockinRoel
Copy link
Contributor

My guess is that the code @chambm mentioned misses the 0 == (Policies::fmtflags::fixed & flags) check that is used below to add the exponent.

@Kojoley
Copy link
Collaborator

Kojoley commented Nov 22, 2021

IIRC from my investigation back then: in any mode the value is rounded, for fmtflags:fixed to the number of digits (it is useful, but counterintuitive, because the standard library iostream does round in fixed mode), the issue is that it is not rounded in all the cases and the workaround in #629 just traded one cases for others. I have a fix but it uses pow10(trunc(floor(log10(abs_n)))) which seems to be pretty heavy on performance and I still does not have a way to prove its correctness for all the cases.

@Kojoley
Copy link
Collaborator

Kojoley commented Nov 22, 2021

Ah, I remembered that my fix also introduces 2 ULP error for max cases in scientific mode which might imply there are should be more, and I did not feel it is an acceptable, so I abandoned the whole idea.

emweb pushed a commit to emweb/wt that referenced this issue Dec 12, 2021
Since Boost 1.76.0 there is a regression in the outputting of numbers:
boostorg/spirit#688

Wt depends on this to serialize doubles to JavaScript.

Until Boost incorporates a fix for this issue we will disable the use of
Spirit to serialize doubles and use sprintf instead for versions of
Boost starting from 1.76.0.

Hopefully it will be fixed in 1.78.0, at which point we can update the
check.
emweb pushed a commit to emweb/wt that referenced this issue Dec 12, 2021
Since Boost 1.76.0 there is a regression in the outputting of numbers:
boostorg/spirit#688

Wt depends on this to serialize doubles to JavaScript.

Until Boost incorporates a fix for this issue we will disable the use of
Spirit to serialize doubles and use sprintf instead for versions of
Boost starting from 1.76.0.

Hopefully it will be fixed in 1.78.0, at which point we can update the
check.
Kojoley pushed a commit that referenced this issue Dec 16, 2021
This fixes issue #688. There's some code that divides the integer part
by 10 and increases dim to compensate for it. This code is not
applicable to fixed notation, however, since dim is disregarded in that
case, causing the result to be off by a factor 10.

We can fix this by checking if we're not using fixed notation first,
before doing the division.

Added a few tests to check for this regression.
@fiesh
Copy link

fiesh commented May 9, 2022

This was fixed with the release of boost 1.79.0 and should therefore be closed I think.

@knogas
Copy link

knogas commented Aug 4, 2022

We've upgraded to Boost 1.79.0 from 1.68.0 and have a unit test failure for the following:

A double of 3.0000000000000001e-06 is being returned as the following string: "3.000000000e-06" whereas we are expecting "3.0e-06" which is what was returned with Boost 1.68.0.

@friedrichatgc
Copy link

Please see also my last comment on #736 which fixed a similar issue but introduced a new issue:
3.1 is output as 3.01000000000000001 if precision is >=16.

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

No branches or pull requests

7 participants