[stdlib] Fix atol() when explicitly setting base 2, 8, and 16 #2650
Conversation
| @@ -248,6 +248,20 @@ fn _atol(str_ref: StringRef, base: Int = 10) raises -> Int: | |||
| start = pos | |||
| break | |||
|
|
|||
| if str_ref[start] == "0" and start + 1 < str_len: | |||
There was a problem hiding this comment.
maybe for the cases of base 2, 8 and 16 you can call _identify_base and if the returned base do not match the expected base than you raise an error?, just to prevent duplicating the logic for the base identification twice
There was a problem hiding this comment.
Thanks for the advice. It seems in case of atol(" 12 ", 8), call _identify_base directly would get a base of 10.
There was a problem hiding this comment.
That's true, which means that in case of 2, 8 and 16, the identified base must be the original base or 10 otherwise you raise an error
There was a problem hiding this comment.
I understand _identify_base function is designed especially for the cases that base is set to 0. So it requires int literals to identify themselves without ambiguity. If an int literal start with '0x', it is recognized as real base 16. Also the same for real base 2 and 8. If an int literal comes without prefix such as '0x' (e.g 'ff' of base 16), it would return real base -1 and raise error. So it might be a little bit difficult to use _identify_base for int literals which base is explicitly set.
There was a problem hiding this comment.
When get a real base -1 returned, we might need to determine whether the int literal is actualy valid if it is explicitly set a base (e.g 'ff' of base 16), or it is totally invalid such as "$%^&!!". So can we just fix this simply?
There was a problem hiding this comment.
For bases 2, 8, and 16 it is ok if the identified base is the original, 10 or -1, if something is wrong atol it's going to fail later.
The idea behind what I said was just to prevent duplicating the logic for the base identifiers (0b, 0B, 0x, ...), but feel free to adapt the code if what I proposed does not work.
There was a problem hiding this comment.
Yes, I understand. I tried to use _identify_base to avoid duplicating the logic, but processing return value -1 is little bit complicated (as said above in case of 'ff' with explicite base 16). So maybe we could simply fix existing problem, then to consider how we can combine the logic into one function. Thanks.
northstreet12
force-pushed
the
nightly
branch
4 times, most recently
from
e11b49f
to
26a8ffe
Compare
|
Hi, any comments? |
Connor is out of office this week, so I'll take a look at this tomorrow (Tuesday) during code reviews. Thanks! |
northstreet12
force-pushed
the
nightly
branch
3 times, most recently
from
15283ad
to
544d829
Compare
Signed-off-by: Luke <leguang.liu@terapines.com>
|
@JoeLoser Hi, can you take a look at this? Just a simple fix :) |
1 similar comment
|
@JoeLoser Hi, can you take a look at this? Just a simple fix :) |
|
!sync |
Yep, sorry I missed this one this week. Just approved and imported the PR. |
modularbot
added
the
imported-internally
|
✅🟣 This contribution has been merged 🟣✅ Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours. We use Copybara to merge external contributions, click here to learn more. |
modularbot
added
the
merged-internally
… 16 (#40634)
[External] [stdlib] Fix atol() when explicitly setting base 2, 8, and 16
Fix `atol()` when explicitly setting base 2, 8, and 16.
```
atol(" 0xff ", 16)
atol(" 0o12 ", 8)
```
Co-authored-by: northstreet12 <leguang.liu@terapines.com>
Closes #2650
MODULAR_ORIG_COMMIT_REV_ID: 1c35383e9a4fe74b60e60be82c9149df611a4ae5
|
Landed in 2ce19d3! Thank you for your contribution 🎉 |
… 16 (#40634)
[External] [stdlib] Fix atol() when explicitly setting base 2, 8, and 16
Fix `atol()` when explicitly setting base 2, 8, and 16.
```
atol(" 0xff ", 16)
atol(" 0o12 ", 8)
```
Co-authored-by: northstreet12 <leguang.liu@terapines.com>
Closes modularml#2650
MODULAR_ORIG_COMMIT_REV_ID: 1c35383e9a4fe74b60e60be82c9149df611a4ae5
… 16 (#40634)
[External] [stdlib] Fix atol() when explicitly setting base 2, 8, and 16
Fix `atol()` when explicitly setting base 2, 8, and 16.
```
atol(" 0xff ", 16)
atol(" 0o12 ", 8)
```
Co-authored-by: northstreet12 <leguang.liu@terapines.com>
Closes modularml#2650
MODULAR_ORIG_COMMIT_REV_ID: 1c35383e9a4fe74b60e60be82c9149df611a4ae5
Signed-off-by: Avinag <udayagiriavinag@gmail.com>
Should atol() be fixed when explicitly setting base 2, 8, and 16?