Opened 11 months ago
Closed 11 months ago
#9231 closed bug (fixed)
7.8 regression in Read instance of Data.Fixed.Pico
Reported by: | leonbaum2 | Owned by: | |
---|---|---|---|
Priority: | highest | Milestone: | 7.8.3 |
Component: | libraries/base | Version: | 7.8.2 |
Keywords: | regression | Cc: | hvr, ekmett, thoughtpolice, igloo, ashley@… |
Operating System: | Unknown/Multiple | Architecture: | Unknown/Multiple |
Type of failure: | None/Unknown | Test Case: | |
Blocked By: | Blocking: | ||
Related Tickets: | #9240 #7483 | Differential Revisions: |
Description
If I run the following with ghc-7.8.2, the runtime system eats up all of my memory and doesn't produce a result:
read "1" :: Data.Fixed.Pico
Change History (12)
comment:1 Changed 11 months ago by hvr
comment:2 Changed 11 months ago by hvr
- Cc thoughtpolice added
- Keywords regression added
- Milestone set to 7.8.3
- Priority changed from normal to highest
comment:3 Changed 11 months ago by hvr
I can't think of any cleaner fix for this other than radically rewriting Data.Fixed or extracting the 10-base exponent via
e = ceiling (logBase 10 (fromInteger r) :: Double)
and having the code read like
convertFixed :: forall a . HasResolution a => Lexeme -> ReadPrec (Fixed a) convertFixed (Number n) | Just (i, f) <- numberToFixed e n = return (fromInteger i + (fromInteger f / fromInteger r)) where r = resolution (undefined :: Fixed a) e = ceiling (logBase 10 (fromInteger r) :: Double) convertFixed _ = pfail
(Note that numberToFixed also expects an exponent rather than a power-of-10)
comment:4 Changed 11 months ago by hvr
The really weird thing though is why the test-case at libraries/base/tests/readFixed001.hs actually works as expected....
comment:5 Changed 11 months ago by simonpj
- Cc igloo ashley@… added
convertFixed was last modified (or written) by Ian Lynagh. Much of the rest of Data.Fixed is by Ashley Yakeley.
I'm copying both, not in a blaming way, but because they would both be well positioned to offer help on fixing.
But others, please go ahead and suggest fixes.
Simon
comment:6 Changed 11 months ago by hvr
I've just submitted https://phabricator.haskell.org/D25 for review to get the ball rolling
comment:7 Changed 11 months ago by Herbert Valerio Riedel <hvr@…>
comment:8 Changed 11 months ago by hvr
- Status changed from new to merge
comment:9 follow-up: ↓ 10 Changed 11 months ago by Ashley Yakeley
Just to clarify, r is not necessarily a power of 10.
comment:10 in reply to: ↑ 9 Changed 11 months ago by hvr
Replying to Ashley Yakeley:
Just to clarify, r is not necessarily a power of 10.
Now that you say it, I had the impression an implicit power-of-10 assumption was baked into Data.Fixed, for instance consider the following weird behaviour (can be observed in GHC 7.6 as well):
Prelude> import Data.Fixed Prelude Data.Fixed> data B7 Prelude Data.Fixed> instance HasResolution B7 where resolution _ = 128 Prelude Data.Fixed> 1.070 :: Fixed B7 1.062 Prelude Data.Fixed> 1.062 :: Fixed B7 1.054 Prelude Data.Fixed> 1.054 :: Fixed B7 1.046 Prelude Data.Fixed> 1.046 :: Fixed B7 1.039
comment:11 Changed 11 months ago by Herbert Valerio Riedel <hvr@…>
comment:12 Changed 11 months ago by hvr
- Resolution set to fixed
- Status changed from merge to closed
The fix for the regression has been merged to ghc-7.8 via [99462b6877308003442942cbddb3296f29cfb9a8/base] and [4254e158e05ac86b922dea6ba3c3c330732d991b/base]. The remaining "read . show == id" has been moved into a ticket of its own (#9240)
The parser code seems totally wrong, e.g. consider
And note that resolution doesn't return an exponent but rather a power of 10.