You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This is not a bug, but a mistake in the code that made me think "how does this work?", and could be a bug waiting to happen. In the code for strconv.ParseComplex() (added in Go 1.15), it sets size to 128 by default (instead of 64), and passes that size to parseFloatPrefix. That function should really receive 64 for this case, but it currently happens to work with anything that's not 32, so this doesn't actually cause a bug. Here's the code for ParseComplex:
And parseFloatPrefix is defined as follows -- it handles anything other than bitSize == 32 as 64, so the invalid size of 128 happens to work:
funcparseFloatPrefix(sstring, bitSizeint) (float64, int, error) {
ifbitSize==32 {
f, n, err:=atof32(s)
returnfloat64(f), n, err
}
returnatof64(s)
}
So the minimal change would be to change the size := 128 to size := 64 in ParseComplex. But there were a couple of other things I saw while in there:
There don't seem to be any tests for ParseComplex with bitSize == 64 (two float32's): https://golang.org/src/strconv/atoc_test.go -- it doesn't seem good that we're not testing the code paths for each size. many of the bitSize 128 tests should work the same (and we could reuse those), but the range/overflow ones will need to be different.
There don't seem to be any tests at all for FormatComplex. I know it's pretty trivially defined in terms of FormatFloat, but there is some logic in it, and we should still have tests to avoid regressions.
It seems to me that parseFloatPrefix (and ParseFloat) should panic if bitSize is anything other than 64 or 128. FormatComplex already does this (code here), and so it seems for consistency (and to catch issues like this one) the other functions should do similar checks. ParseFloat and FormatFloat are clearly documented to only allow 32 and 64. Alternatively they could return an error for this case, but it seems to me errors should only be returned for bad input, not invalid types, so the panic is the right thing to do.
If folks agree, I'd be happy to put up a CL for these changes.
The text was updated successfully, but these errors were encountered:
I think it would be strange for ParseFloat to panic when it has an error return specified. Reporting errors in two different ways would just lead to confusion.
FormatFloat would need to panic, though (or return a "bad bit size" string).
@griesemer Thanks. I'm not sure how the Go project uses GitHub "assignees" -- but I'm hoping the fact you assigned yourself doesn't mean I can't submit a CL. :-) So I've submitted https://go-review.googlesource.com/c/go/+/248219/ with this fix and the addition of some tests -- see what you think. Details in commit message.
This is not a bug, but a mistake in the code that made me think "how does this work?", and could be a bug waiting to happen. In the code for
strconv.ParseComplex()
(added in Go 1.15), it setssize
to 128 by default (instead of 64), and passes that size toparseFloatPrefix
. That function should really receive 64 for this case, but it currently happens to work with anything that's not 32, so this doesn't actually cause a bug. Here's the code forParseComplex
:And
parseFloatPrefix
is defined as follows -- it handles anything other thanbitSize == 32
as 64, so the invalid size of 128 happens to work:So the minimal change would be to change the
size := 128
tosize := 64
inParseComplex
. But there were a couple of other things I saw while in there:ParseComplex
withbitSize == 64
(two float32's): https://golang.org/src/strconv/atoc_test.go -- it doesn't seem good that we're not testing the code paths for each size. many of the bitSize 128 tests should work the same (and we could reuse those), but the range/overflow ones will need to be different.FormatComplex
. I know it's pretty trivially defined in terms ofFormatFloat
, but there is some logic in it, and we should still have tests to avoid regressions.parseFloatPrefix
(andParseFloat
) should panic if bitSize is anything other than 64 or 128.FormatComplex
already does this (code here), and so it seems for consistency (and to catch issues like this one) the other functions should do similar checks.ParseFloat
andFormatFloat
are clearly documented to only allow 32 and 64. Alternatively they could return an error for this case, but it seems to me errors should only be returned for bad input, not invalid types, so the panic is the right thing to do.If folks agree, I'd be happy to put up a CL for these changes.
The text was updated successfully, but these errors were encountered: