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

1.0#... does not work and never worked but has code for it #415

Closed
Kojoley opened this issue Nov 2, 2018 · 19 comments

Comments

Projects
None yet
4 participants
@Kojoley
Copy link
Collaborator

commented Nov 2, 2018

Update: Was already reported on trac 5 years ago https://svn.boost.org/trac10/ticket/8699.

Real parsers fail on infinity values in form 1.0#INF and it never ever worked (in both Qi and X3). The only mention that Spirit parses things like that was buried in RealPolicies documentation.

The current code

else if (traits::is_equal_to_one(acc_n))
{
// There is a chance of having to parse one of the 1.0#...
// styles some implementations use for representing NaN or Inf.
// Check whether the number to parse is a NaN or Inf
if (p.parse_nan(first, last, n) ||
p.parse_inf(first, last, n))
{
// If we got a negative sign, negate the number
traits::assign_to(traits::negate(neg, n), attr);
return true; // got a NaN or Inf, return immediately
}
n = static_cast<T>(acc_n);
}

accepts values in undocumented form like 1INF and 1.INF, but it fails with mantissa - i.e., 1.0INF.

Fixing it to allow 1.0#INF scares me because it will be backward-incompatible change for users that rely on current behavior, it will break parsers like double_ >> "#...". I think this code need to be removed, as it never accepted values in documented form and has undocumented implications. Removing the code will make real parser slightly faster and will remove custom real requirement for traits::is_equal_to_one (partly solves the problem mentioned in #163).

        BOOST_TEST(test("1#INF", double_));    // Fails
        BOOST_TEST(test("1.#INF", double_));   // Fails
        BOOST_TEST(test("1.0#INF", double_));  // Fails
        BOOST_TEST(test("1INF", double_));     // Ok, but must not
        BOOST_TEST(test("1.INF", double_));    // Ok, but must not
        BOOST_TEST(test("1.0INF", double_));   // Fails
@djowel

This comment has been minimized.

Copy link
Member

commented Nov 2, 2018

@hkaiser what do you think about this?

@djowel

This comment has been minimized.

Copy link
Member

commented Nov 3, 2018

BTW, didn't we have a test for this?

@Kojoley

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 3, 2018

I did not find any.

@Kojoley Kojoley closed this Nov 3, 2018

@Kojoley Kojoley reopened this Nov 3, 2018

@djowel

This comment has been minimized.

Copy link
Member

commented Nov 3, 2018

OK, well, I am for removing it. But let's wait a bit for @hkaiser because he was the one (IIRC) who added the infinity stuff. I might be missing something.

@Kojoley

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 3, 2018

Yes, sure, there is no hurry with this as we already in process of 1.69 beta and this is kind of a breaking change.

@hkaiser

This comment has been minimized.

Copy link
Collaborator

commented Nov 4, 2018

@djowel did I add that to Qi? Doh! I remember adding it to Karma, though. But seriously, I'd rather fix it and document it well. If we're going to remove things from Qi we should remove it from Karma as well; and people might depend on it there...

@Kojoley

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 4, 2018

@djowel did I add that to Qi? Doh!

The file appeared with this and it was added with a single big commit that brought the Spirit V2 ffd0cc1 and this part did not change since then, so it is hard to tell who wrote this from the commit history :-)

I remember adding it to Karma, though

I cannot find anything about actual nan and inf representation format in Karma. The documentation only mention nan and inf functions in real policies https://www.boost.org/doc/libs/1_68_0/libs/spirit/doc/html/spirit/karma/reference/numeric/real_number.html. The default real policy code

template <typename CharEncoding, typename Tag, typename OutputIterator>
static bool nan (OutputIterator& sink, T n, bool force_sign)
{
return sign_inserter::call(
sink, false, traits::test_negative(n), force_sign) &&
string_inserter<CharEncoding, Tag>::call(sink, "nan");
}
template <typename CharEncoding, typename Tag, typename OutputIterator>
static bool inf (OutputIterator& sink, T n, bool force_sign)
{
return sign_inserter::call(
sink, false, traits::test_negative(n), force_sign) &&
string_inserter<CharEncoding, Tag>::call(sink, "inf");
}
clearly prints "nan" and "inf". The tests only test [+-]?(nan|inf), again I cannot find anything about 1.0#... in Karma.

I'd rather fix it and document it well

As I mentioned above the fix will be a breaking change for Qi users. The fix will also require make something with traits::is_equal_to_one(val) as the current implementation (val == 1.0) is unreliable and platform/toolset dependent and the fix will be costly.

@hkaiser

This comment has been minimized.

Copy link
Collaborator

commented Nov 4, 2018

@Kojoley ok, I apparently misremember things. Let's remove the parsers if Karma does not expose 1#INF.

@Kojoley

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 22, 2018

Found 5 year old trac ticket about exactly this issue https://svn.boost.org/trac10/ticket/8699.

@djowel

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

Let's do it. I trust your and @K-ballo 's judgment.

@hkaiser

This comment has been minimized.

Copy link
Collaborator

commented Nov 22, 2018

Hold on, apparently the fix is trivial (see @K-ballo's patch).

@Kojoley

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 22, 2018

I know that the fix is trivial, but I am sure it is better just to remove the code. Reread my arguments again, they are all still valid.

@hkaiser

This comment has been minimized.

Copy link
Collaborator

commented Nov 22, 2018

Sure, you decide. ...just wanted to make sure people realize its trivial to fix.

@Kojoley

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 22, 2018

Plus, no one commented that issue for 5 years, so I believe nobody needs that.

@djowel

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

Let's see if @K-ballo will comment. I'm OK either way.

@K-ballo

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

Plus, no one commented that issue for 5 years, so I believe nobody needs that.

I had a need for that, hence why I created that issue and worked on implementing a fix. I have since moved away from spirit, so it no longer affects me.

@djowel

This comment has been minimized.

Copy link
Member

commented Nov 22, 2018

@K-ballo do you remember the-use case you had when you needed it?

@Kojoley

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 22, 2018

I am also interested in use cases, because I am not against having such a parser, it just should not be general-purpose in my opinion. Currently it is designed in such way that parse_inf/parse_nan called twice without providing the user with ability to turn off just this syntax. So if for example a user makes parse_nan that handles he will get not only +∞ and -∞, but also 1.0#∞ and so on.

@K-ballo

This comment has been minimized.

Copy link
Member

commented Nov 24, 2018

@K-ballo do you remember the-use case you had when you needed it?

Unfortunately I no longer recall the details, it's been years.

Kojoley added a commit to Kojoley/spirit that referenced this issue Feb 9, 2019

Remove broken 1.0#INF parser
It was poorly documented and never worked correctly. None of atof, strtof, and
input streams are accepting such values. Fixing it would be a bigger breaking
change than removing it.

Fixes boostorg#415, https://svn.boost.org/trac10/ticket/8699

Kojoley added a commit to Kojoley/spirit that referenced this issue Feb 9, 2019

Remove broken 1.0#INF parser
It was poorly documented and never worked correctly. None of atof, strtof, and
input streams are accepting such values. Fixing it would be a bigger breaking
change than removing it.

Fixes boostorg#415, addresses boostorg#163 and https://svn.boost.org/trac10/ticket/8699

@Kojoley Kojoley closed this in #458 Feb 9, 2019

Kojoley added a commit that referenced this issue Feb 9, 2019

Remove broken 1.0#INF parser
It was poorly documented and never worked correctly. None of atof, strtof, and
input streams are accepting such values. Fixing it would be a bigger breaking
change than removing it.

Fixes #415, addresses #163 and https://svn.boost.org/trac10/ticket/8699
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.