Page MenuHomePhabricator

Fix PR #25788: parsing of floating-point constants on non-C locales
ClosedPublic

Authored by pitrou on Dec 9 2015, 5:19 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

pitrou updated this revision to Diff 42286.Dec 9 2015, 5:19 AM
pitrou retitled this revision from to Fix PR #25788: parsing of floating-point constants on non-C locales.
pitrou updated this object.
pitrou added a subscriber: llvm-commits.
hfinkel added inline comments.
unittests/AsmParser/AsmParserTest.cpp
90 ↗(On Diff #42286)

I don't think we can use this here because it is not thread safe (and gtest might run multiple tests in parallel).

I'm fine with this going in without a regression test if there's no reasonable way to construct one.

pitrou added inline comments.Dec 9 2015, 11:37 AM
unittests/AsmParser/AsmParserTest.cpp
90 ↗(On Diff #42286)

Yes, it's the only way of testing I could think about. Unfortunately the Unix locale system isn't very flexible...

I'll strip the test code and re-submit if nobody else suggests an idea.

pitrou updated this revision to Diff 43028.Dec 16 2015, 10:28 AM

Here is an updated patch without the locale-changing test.

pitrou added a comment.Jan 7 2016, 5:51 AM

Just a ping. I'm not sure there's anything holding this patch?

Ping again :-) This is bitting our users on a regular basis: https://github.com/numba/numba/issues/1656

I think we'd want APFloat::convertFromDecimalString with rmNearestTiesToEven seeing as how we don't expect to see -nan and the like.

I think we'd want APFloat::convertFromDecimalString with rmNearestTiesToEven seeing as how we don't expect to see -nan and the like.

I've looked at this and there are a couple issues:

  • convertFromDecimalString() is private
  • convertFromDecimalString() doesn't handle leading sign

Also, due to the lexing that occurs before calling APFloat() here, I don't think it can actually be called with -nan and friends. I'll upload an updated patch as it turns out the one here doesn't apply anymore.

pitrou updated this revision to Diff 46233.Jan 28 2016, 12:13 AM

Updated against git master.

majnemer accepted this revision.Mar 15 2016, 1:27 PM
majnemer added a reviewer: majnemer.

LGTM

This revision is now accepted and ready to land.Mar 15 2016, 1:27 PM
pitrou added a comment.Jun 2 2016, 8:22 AM

Sorry for the ping, but this patch has been marked "accepted and ready to land", and I can't see it on master. Is anything needed on my part?

meadori added a subscriber: meadori.Jun 2 2016, 9:30 AM

Do you have commit rights?

If not, then I will commit it for you today.

Thanks, Meador! No, I don't.

This revision was automatically updated to reflect the committed changes.