Skip to content

Conversation

@nitely
Copy link
Contributor

@nitely nitely commented Sep 4, 2025

Fixes #77

Changes:

  • Implements compile-time/vm support for memory inputs/outputs
  • Changes writeText for floats so the output is the same across nim targets and versions. However, for float32 the output should now be the same as for float64. Nim +2.2.2 made this the default. The alternative is to keep the current behavior, but the result differs for the VM and across Nim versions, for example 1.23 is printed as 1.230000019073486 at runtime (sprintf) in Nim < 2.2.2, 1.23 in Nim >= 2.2.2, and 1.2300000190734863 (note the extra digit) at comptime (after cast[float32](cast[uint32](val)) the float32 val).

I checked json and cbor serializers passes most tests at comptime with these changes, the few that fail are unrelated to faststreams.

Tested for regressions here:

@nitely nitely marked this pull request as ready for review September 8, 2025 22:00
@nitely nitely requested review from arnetheduck and jangko September 8, 2025 22:00

template writeText*(s: OutputStream, val: auto) =
write s, $val
when val is SomeFloat:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not leave it as an overload?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overload doesn't work in Nim 2.0.16 due to a Nim regression nim-lang/Nim#25166

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

welcome to nim development ;) we note bugs in the code with # TODO https://github.com/nim-lang/Nim/issues/25166 so we can track them down later, at the point where they're relevant

@arnetheduck
Copy link
Member

float roundtrip format lgtm - indeed, better to have it consistent and above all, round-trippable for the intended use case.

@nitely nitely merged commit 8a94d6f into status-im:master Sep 12, 2025
20 checks passed
@arnetheduck
Copy link
Member

hm, also, can replace the custom code in toml-serialization now?

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.

Add compile-time support for reading/writing simple streams

2 participants