flyx / NimYAML

YAML implementation for Nim
https://nimyaml.org
Other
186 stars 36 forks source link

BUG: loading to float32 generates an error #124

Closed ggxa closed 2 years ago

ggxa commented 2 years ago

Reduced reproduction code:

import streams
import yaml

type x = object
  y : float32

var o = x(y: 0)

let w = newFileStream("o.yaml", fmWrite)
dump(o, w)
w.close()

let r = newFileStream("o.yaml")
load(r, o)
echo o

serialization.nim(284, 53) Error: for a 'var' type a variable needs to be passed; but 'BiggestFloat(result)' is immutable

Suggested fix:

@@ -281,9 +281,13 @@ proc constructObject*[T: float|float32|float64](
     let hint = guessType(item.scalarContent)
     case hint
     of yTypeFloat:
-      discard parseBiggestFloat(item.scalarContent, result)
+      var res: BiggestFloat
+      discard parseBiggestFloat(item.scalarContent, res)
+      result = res
     of yTypeInteger:
-      discard parseBiggestFloat(item.scalarContent, result)
+      var res: BiggestFloat
+      discard parseBiggestFloat(item.scalarContent, res)
+      result = res
     of yTypeFloatInf:
         if item.scalarContent[0] == '-': result = NegInf
         else: result = Inf 

The conversion from BiggestFloat which is float64 usually to smaller floats like float32 could result in loss of data but that is expected I guess.

flyx commented 2 years ago

Data loss is always expected for floating point literals, since you are never forced to input only values that are exactly representable. So this fix is perfectly fine. Want to make a PR?

ggxa commented 2 years ago

Could you please do it yourself. I don't want to deal with github. This is a throwaway account just to report some issues.

Also can you check my other bug report about loading zero to uint. A while ago a similar fix was applied for int, it should also be done for uint.

Thank you!

flyx commented 2 years ago

Thank you for the suggested fixes, your issues have been addressed.

The simpler you make it for me, the faster I can react; but you are of course free to suggest fixes like this.