Error in from_str_radix if radix > 18; avoid unfortunate interaction #90
Conversation
|
Where and if to panic and where to check for the error involves some nonobvious choices. I'd prefer if "radix <= 18" is a precondition to calling "from_str_generic", i.e that this function doesn't have to care for possible 'i' as a digit and so on. I'd also be fine to just have radix <= 16 as the requirement if that just seems easier to understand. One alternative not explored here is to invent a new format for complex numbers - and use that for radix > 18. I don't have a great suggestion, but imagine if it was something like Playground link - showcase weird parses As an example of a weird parse, "1i" is 38 as a real number in the type i32 with radix 20. But "1i + 1i" parses as the complex 38 + i in type |
bluss
force-pushed
the
complex-1ij-plus-1ij
branch
3 times, most recently
from
66afe4c to
fa553ec
Compare
|
That's an unfortunate oversight, but I agree that an error is the most reasonable thing we can do. |
| /// `radix` must be <= 18; Larger radix would include *i* and *j* as digits, | ||
| /// which can not be supported. | ||
| /// | ||
| /// The conversion returns an error if 18 <= radix <= 36; it panics if radix > 36. |
There was a problem hiding this comment.
Let's note that error/panic conditions are inherited from T as well.
There was a problem hiding this comment.
Ah, good point. I don't feel like I have the best formulations today, but I gave it a shot :)
bluss
force-pushed
the
complex-1ij-plus-1ij
branch
6 times, most recently
from
ecfd081 to
ada1db0
Compare
Radix 19 and 20 introduce 'i' and 'j' as digits and this conflicts with
how complex are parsed. Before this fix, it could parse
`Num::from_str_radix("1ij + 1ij", 20)` in a pretty surprising way.
The `Num` docs already describe a rule for our situation - we might not
support every radix in our implementation; we can return an error in
this case instead.
A new (internal) error kind is introduced for this.
It is necessary to preserve the previous panicking behaviour for
radix > 36 (this is what primitive types do - but other types are
allowed to do differently, including never panicking).
bluss
force-pushed
the
complex-1ij-plus-1ij
branch
from
ada1db0 to
978bff0
Compare
|
So I'm curious, how did you come to realize this? If it was in real use, then someone really did want a larger radix? |
|
Oh, for some reason I was reading the from_str implementation and looked at how it handles i vs j (it looks hacky, but seems to cover all the cases). So I just noticed it, and I don't know any use case for this. Instead of an annoying bug report I wrote a PR directly. I have a feeling that the "<= 18" limit is irregular and it would seem better to only support <= 16 |
|
bors r+ |
|
Build succeeded: |
from_str_radix: Error if radix is > 18
Radix 19 and 20 introduce 'i' and 'j' as digits and this conflicts with
how complex are parsed. Before this fix, it could parse
Num::from_str_radix("1ij + 1ij", 20)in a pretty surprising way.The
Numdocs already describe a rule for our situation - we might notsupport every radix in our implementation; we can return an error in
this case instead.
A new (internal) error kind is introduced for this.
It is necessary to preserve the previous panicking behaviour(?) for
radix > 36 (this is what primitive types do - but other types are
allowed to do differently, including never panicking).