Skip to content

Conversation

Araq
Copy link
Member

@Araq Araq commented Jul 16, 2021

No description provided.

@Araq Araq merged commit 25efb53 into devel Jul 16, 2021
@Araq Araq deleted the araq-undo-dragonbox branch July 16, 2021 16:29
@timotheecour
Copy link
Member

timotheecour commented Jul 16, 2021

@Araq this is a bad PR, for many reasons:

  • it was merged without letting time for discussion which would've shown the flaws in this appproach
  • users now have to choose between dropping roundtrip floats (which was a big feature of 1.6) or having broken semantics where you introduce an inconsistency between RT and VM or const folded expressions:

example 1

proc main(): string =
  let a = 0.1+0.2
  $a
static: echo main()
echo main()

nim r -d:nimFpRoundtrips main

0.3
0.30000000000000004

example 2

const a = 0.1+0.2
echo ($a, a)

nim r -d:nimFpRoundtrips main
("0.3", 0.30000000000000004)

example 3

const a1 = 0.1+0.2
let a2 = a1
doAssert a1 != 0.3 # just to be clear
echo [$a1, $a2]

nim r -d:nimFpRoundtrips main
["0.3", "0.30000000000000004"]

All this does is break roundtrip float and add complexity to the support matrix in nim libraries, creating more incompatibilities.

It also prevents users of 1.6 from having roundtrip floats at CT.

So, no, this doesn't make dragonbox opt-in, it introduces an opt-in flag for a now-broken feature.

And when that flag becomes opt-out, it still won't be compatible with a program that was compiled with -d:nimFpRoundtrips because of the above inconsistencies!

@kaushalmodi
Copy link
Contributor

I was as well surprised seeing this get merged so soon too. Was there a bug report that inspired this PR?

@timotheecour
Copy link
Member

timotheecour commented Jul 20, 2021

I'm fixing those issues in #18531 and #18566

@timotheecour timotheecour removed the TODO: followup needed remove tag once fixed or tracked elsewhere label Jul 23, 2021
PMunch pushed a commit to PMunch/Nim that referenced this pull request Mar 28, 2022
* make dragonbox opt-in via -d:nimFpRoundtrips

* make tests green again

* make tests green again
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

Successfully merging this pull request may close these issues.

3 participants