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

[stdlib] Refactor atol: StringRef to StringSlice and reusability #3207

Closed
wants to merge 2 commits into from

Conversation

jjvraw
Copy link
Contributor

@jjvraw jjvraw commented Jul 9, 2024

This pull request modularises previously inline code, assisting in addressing issue #2639 to allow stol function to reuse some of the functionality. These changes, initially part of PR #3178, have been separated for easier review.

The refactoring extracts _handle_base_prefix & _trim_and_handle_sign. Additionally, the docstring has been revised for clarity.

Alongside these changes, the respective function signatures have been updated to move away from StringRef to StringSlice.

There was also a small FIXME to convert from var to alias, which is now seems possible.

@jjvraw jjvraw requested a review from a team as a code owner July 9, 2024 21:21
@jjvraw jjvraw marked this pull request as draft July 12, 2024 15:16
@jjvraw jjvraw changed the title [stdlib] Refactor atol for reusability [stdlib] Refactor atol: StringRef to StringSlice and reusability Jul 13, 2024
@jjvraw jjvraw force-pushed the refact/atol branch 3 times, most recently from 5b8b424 to 05740d2 Compare July 18, 2024 12:02
@jjvraw jjvraw marked this pull request as ready for review July 18, 2024 12:09
@jjvraw
Copy link
Contributor Author

jjvraw commented Jul 18, 2024

Sorry for the ping, @JoeLoser, but since you recently reviewed atol in #3180, perhaps you could review this as well? The idea is to refactor this for the introduction of stol as per issue #2639. I'll then merge PR #3178 with these changes and make the appropriate changes :)

If not, no problem!

@jjvraw jjvraw changed the title [stdlib] Refactor atol: StringRef to StringSlice and reusability [stdlib] Refactor atol: StringRef to StringSlice and reusability Jul 24, 2024
@JoeLoser JoeLoser self-assigned this Aug 19, 2024
@JoeLoser
Copy link
Collaborator

Sorry for the ping, @JoeLoser, but since you recently reviewed atol in #3180, perhaps you could review this as well? The idea is to refactor this for the introduction of stol as per issue #2639. I'll then merge PR #3178 with these changes and make the appropriate changes :)

If not, no problem!

So sorry this one fell off my radar and I'm just seeing this again now. I'm still +1 for the change. Do you mind rebasing? I know @martinvuyk has been working a lot in this area recently, so things may have changed quite a bit as we've been moving off of StringRef and over to StringSlice in several places.

Copy link
Contributor

@martinvuyk martinvuyk left a comment

Choose a reason for hiding this comment

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

I haven't touched anything around atof and atol since I knew other people were working on it. Viewing the git blame it seems like this hasn't been touched since more than 6 months ago, so I think this will have trivial conflicts. @jjvraw Thanks again for working on this :)

else:
start = pos
break
var str_len = len(str_slice)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var str_len = len(str_slice)
var str_len = str_slice.byte_length()

fn _identify_base(str_ref: StringRef, start: Int) -> Tuple[Int, Int]:
var length = len(str_ref)
fn _identify_base(str_slice: StringSlice, start: Int) -> Tuple[Int, Int]:
var length = len(str_slice)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
var length = len(str_slice)
var length = str_slice.byte_length()

- StringRef to StringSlice
- Introduce _handle_base_prefix & _trim_and_handle_sign
- Changed var to alias

Signed-off-by: Joshua James Venter <[email protected]>
Signed-off-by: Joshua James Venter <[email protected]>
@JoeLoser
Copy link
Collaborator

!sync

@modularbot modularbot added the imported-internally Signals that a given pull request has been imported internally. label Nov 21, 2024
@modularbot
Copy link
Collaborator

✅🟣 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 modularbot added the merged-internally Indicates that this pull request has been merged internally label Nov 21, 2024
@modularbot
Copy link
Collaborator

Landed in be09f29! Thank you for your contribution 🎉

@modularbot modularbot added the merged-externally Merged externally in public mojo repo label Nov 23, 2024
modularbot pushed a commit that referenced this pull request Nov 23, 2024
… reusability (#51354)

[External] [stdlib] Refactor `atol`: `StringRef` to `StringSlice` and
reusability

This pull request modularises previously inline code, assisting in
addressing issue #2639 to allow
`stol` function to reuse some of the functionality. These changes,
initially part of #3178, have been
separated for easier review.

The refactoring extracts `_handle_base_prefix` &
`_trim_and_handle_sign`. Additionally, the docstring has been revised
for clarity.

Alongside these changes, the respective function signatures have been
updated to move away from `StringRef` to `StringSlice`.

There was also a small `FIXME` to convert from `var` to `alias`, which
is now seems possible.

Co-authored-by: Joshua James Venter <[email protected]>
Closes #3207
MODULAR_ORIG_COMMIT_REV_ID: e733ed473f086abe6fc5acccae711e0114c1093d
@modularbot modularbot closed this Nov 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imported-internally Signals that a given pull request has been imported internally. merged-externally Merged externally in public mojo repo merged-internally Indicates that this pull request has been merged internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants