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

cue: float64 support for zero #1670

Closed
wants to merge 1 commit into from
Closed

Conversation

kcburge
Copy link
Contributor

@kcburge kcburge commented Apr 26, 2022

Fix the error when attempting to use strconv.FormatFloat with a zero value:

cannot use 0.0 (type float) as float64 in argument 0 to strconv.FormatFloat: value was rounded down

The tests for maximum and minimum in Float64() do not allow for zero. The below check incorrectly sees zero as smaller, because the Sign() of zero (0) is less than the Sign() of smallestPosFloat64 (1). So, it was clear this logic was not accounting for zero.

	if n.X.Cmp(smallestPosFloat64) == -1 {

closes #1669

Signed-off-by: Kevin Burge kcburge@pm.me

@kcburge kcburge requested a review from cueckoo as a code owner April 26, 2022 23:52
@myitcv myitcv requested a review from mpvl April 27, 2022 09:07
@mpvl mpvl changed the title Float64 support for zero cue: float64 support for zero Apr 27, 2022
@mpvl
Copy link
Member

mpvl commented Apr 27, 2022

Good catch.

@myitcv
Copy link
Member

myitcv commented Apr 28, 2022

I will leave @mpvl to review the change itself, @kcburge, but just a comment on the DCO. The only requirement to assert the DCO is to include the trailer Signed-off-by in the commit message (per the contribution guide). So one change that will be required here is to remove the DCO text itself from the commit message.

Copy link
Member

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

SGTM. I agree with Marcel - nice catch!

@myitcv I'm actually not sure why the DCO check is failing. Its error says that all commits must have the header, and I see the header in the only commit in this PR. #1709 looks the same and it's green, for example.

@myitcv
Copy link
Member

myitcv commented May 16, 2022

@mvdan strange. Not sure what's going on here, because it looks ok to me. Indeed git can also log the trailer:

git log --pretty='%h %an %(trailers:key=Signed-off-by)'

@kcburge - please can you tweak the first line of the commit message per @mpvl's change to the title of this PR?

cue: float64 support for zero

This is inline with the contribution guide on commit messages.

Perhaps a force-push on the fixed commit message will also fix the DCO check... we'll see.

@mvdan
Copy link
Member

mvdan commented May 31, 2022

@kcburge friendly ping about Paul's comment above, when you have the time :) That way we can import this change into Gerrit and merge it.

If you don't have the time to do that in the next week or two, no worries - I can always take over this change for you if you'd prefer.

@curio77
Copy link

curio77 commented Feb 21, 2023

Any update on this? Honestly, as nice as the idea/vision of CUE is, with issues like this it comes across as very flimsy and unusable in practice. 😔

@mvdan mvdan force-pushed the float-zero branch 2 times, most recently from 8847831 to c750c6f Compare April 3, 2023 16:49
@mvdan
Copy link
Member

mvdan commented Apr 3, 2023

Figured out the DCO error; the commit author had the email kevin.burge@nice.com, but the DCO had kcburge@pm.me. The author seems to use the latter for his open source commits, and the signature is what matters more as far as I can tell. We haven't head from Kevin in a while, and we want this fix to be merged, so I've gone ahead and pushed to his fork branch to rebase the change and fix up the commit author and message.

Fix the error when attempting to use strconv.FormatFloat with a zero value:

	cannot use 0.0 (type float) as float64 in argument 0 to strconv.FormatFloat: value was rounded down

The tests for maximum and minimum in ValueFloat64 do not allow for zero.
The below check incorrectly sees zero as smaller, because the Sign of
zero (0) is less than the Sign of smallestPosFloat64 (1). So, it was
clear this logic was not accounting for zero.

		if n.X.Cmp(smallestPosFloat64) == -1 {

Fixes cue-lang#1669.

Signed-off-by: Kevin Burge <kcburge@pm.me>
Change-Id: I16a9fde39c36c519501c6cb3f87470a7f0cbab36
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.

strconv.FormatFloat fails with zero (0.0)
5 participants