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

Overhaul Data.ByteString.Builder.RealFloat #636

Open
BebeSparkelSparkel opened this issue Dec 28, 2023 · 5 comments
Open

Overhaul Data.ByteString.Builder.RealFloat #636

BebeSparkelSparkel opened this issue Dec 28, 2023 · 5 comments

Comments

@BebeSparkelSparkel
Copy link
Contributor

I have created the following PRs but they seem to be intermingled and perhaps a larger pull would be better
#632
#633
#634

I propose the following

  1. combine the logic of formatFloat and formatDouble into formatFloating :: FloatFormat -> a -> Builder since they have the same logic and use classes to get their specific functions based on the floating type. Continuing exporting the formatFloat and formatDouble interface functions for compatibility.
  2. Replace the constructor of FloatFormat to the constructors of FormatMode and remove the type FormatMode because the precision (Maybe Int) is not used and can be included as a parameter of the constructors
  3. FScientific should have a Char for specifying a lower or upper E
  4. FStandard should have a Maybe Int for the precision
  5. FGeneric should have a Char for specifying a lower or upper E, and two Ints for the inclusive exponent range for printing the standard notation
  6. Allow custom strings for special values +-Infinity +-0 NaN with data SpecialStrings = {..}
  7. remove specialStr and use toCharsNonNumbersAndZero instead because they duplicate logic and it is faster to evaluate
  8. I am unsure if this is optimized away but the case statement that selects the format should not be executed for every floating number to be printed. If not, the \f -> ... should be defined after the case statement and not before.

None of these changes should cause changes to the existing interface.

@clyring
Copy link
Member

clyring commented Jan 5, 2024

  • (1) I'm also not very happy with the amount of un-shared logic between the Float and Double-formatting code. But using classes to de-duplicate this logic may require considerable care to make sure that all of the actual class-dictionaries are specialized away and all of the unboxing we expect actually happens. (Backpack isn't really an option for us; abusing CPP is another possibility.) I don't expect the cost/benefit to be favorable unless you are rewriting large chunks, but if you want to work on this I won't stop you.
  • (2) sounds sensible.
  • (3), (5) I can see some value in selecting between 1e40 and 1E40. But Char is not appropriate here: Word8 for a single byte, or perhaps Builder is less ugly, or perhaps a Bool indicating "isUppercase."
  • (4) is implied by (2).
  • (6) It's very easy for a user to check for these and format them specially if they have some particular reason to. It's probably not worth expanding our API for.
  • (7) These are not duplicates because of 0.0e0 versus 0.0.
  • (8) GHC is usually pretty cavalier about eta-expanding through such case statements anyway. (This can hurt when it results in a real PAP that is called several times but I expect that to be very uncommon for formatDouble.)

(Sorry for taking so long to respond.)

@BebeSparkelSparkel
Copy link
Contributor Author

Builder is an intriguing idea instead of char. I'll have to think about that since it does allow for more than one Word8 to be inserted.

I think 6 and 7 are somewhat related and it seems worth it to consolidate for the faster determination of special values for the standard case.

@BebeSparkelSparkel

This comment was marked as outdated.

@clyring
Copy link
Member

clyring commented Jan 24, 2024

I see you're doing a lot of work in various PRs in this area. Please let me know when each one is ready for review, and which ones to start with/prioritize.

@BebeSparkelSparkel
Copy link
Contributor Author

@clyring Please focus on

  1. Better Tests and Benchmarks for Float and Double Builders #638 -- Adds tests that will fail without Overhaul realfloat #637
  2. Overhaul realfloat #637 -- the big one

The others can come later.

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

2 participants