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.

Sign up for GitHub

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

Jump to bottom

datamancer tests doesn't work with -d:nimPreviewFloatRoundtrip #72

Closed
ringabout opened this issue Oct 3, 2024 · 3 comments
Closed

datamancer tests doesn't work with -d:nimPreviewFloatRoundtrip #72

ringabout opened this issue Oct 3, 2024 · 3 comments

Comments

@ringabout
Copy link
Contributor

Hi, in nim-lang/Nim#24217, I'm trying to make -d:nimPreviewFloatRoundtrip the default. It seems to break some float tests:

/home/runner/work/Nim/Nim/pkgstemp/datamancer/tests/testParse.nim(124, 31): Check failed: classify(floatBuf) == fcNormal
2024-10-03T12:04:21.2772069Z     classify(floatBuf) was fcNan
2024-10-03T12:04:21.2772195Z     fcNormal was fcNormal
2024-10-03T12:04:21.2772436Z   [FAILED] Valid float values with separator

ref https://github.com/nim-lang/Nim/actions/runs/11161348816/job/31023719357?pr=24217
@Vindaar
Copy link
Member

Vindaar commented Oct 13, 2024

Hey!

Sorry, didn't see this before. I just noticed this myself while working on #73.

I fixed it there. The reason was that I specifically had a when defined(nimPreviewFloatRoundtrip) case in there. With it becoming the default, it now chose the old, wrong branch of the code and not the correct one for it.

@Vindaar Vindaar closed this as completed Oct 13, 2024
@ringabout
Copy link
Contributor Author

@Vindaar Awesome!

@ringabout
Copy link
Contributor Author

When nim-lang/Nim#24300 is merged, nimHasDefaultFloatRoundtrip will be available for conditional testing

@ringabout ringabout mentioned this issue Oct 13, 2024
Merged
ringabout added a commit to nim-lang/Nim that referenced this issue Oct 14, 2024
@ringabout
define -d:nimHasDefaultFloatRoundtrip and enable datamancer (#24300)
d4b9c14 
ref SciNim/Datamancer#73
ref SciNim/Datamancer#72
narimiran pushed a commit to nim-lang/Nim that referenced this issue Jan 14, 2025
@ringabout @narimiran
define -d:nimHasDefaultFloatRoundtrip and enable datamancer (#24300)
e3a8d98 
ref SciNim/Datamancer#73
ref SciNim/Datamancer#72

(cherry picked from commit d4b9c14)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@Vindaar @ringabout