boostorg / spirit

Boost.org spirit module
http://boost.org/libs/spirit
393 stars 161 forks source link

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

Closed Kojoley closed 5 years ago

Kojoley commented 6 years ago

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 https://github.com/boostorg/spirit/blob/cf631bf68f0efdeabdfba24a0dea515ece255298/include/boost/spirit/home/qi/numeric/detail/real_impl.hpp#L300-L315 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 commented 6 years ago

@hkaiser what do you think about this?

djowel commented 6 years ago

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

Kojoley commented 6 years ago

I did not find any.

djowel commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 ffd0cc1001163ba59eddd7cbd248862b5162c2d4 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 https://github.com/boostorg/spirit/blob/cf631bf68f0efdeabdfba24a0dea515ece255298/include/boost/spirit/home/karma/numeric/real_policies.hpp#L316-L330 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 commented 6 years ago

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

Kojoley commented 5 years ago

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

djowel commented 5 years ago

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

hkaiser commented 5 years ago

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

Kojoley commented 5 years ago

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 commented 5 years ago

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

Kojoley commented 5 years ago

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

djowel commented 5 years ago

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

K-ballo commented 5 years ago

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 commented 5 years ago

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

Kojoley commented 5 years ago

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 commented 5 years ago

@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.