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.

Sign up for GitHub

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

Jump to bottom

{Q,Z},of_string accept underscores #75

Merged
merged 5 commits into from Oct 13, 2020
Merged

{Q,Z},of_string accept underscores #75

merged 5 commits into from Oct 13, 2020

Conversation

hhugo
Copy link
Contributor

@hhugo hhugo commented Sep 21, 2020 •
edited

close #71

@hhugo hhugo mentioned this pull request Sep 21, 2020
Merged
@hhugo
Copy link
Contributor Author

hhugo commented Sep 21, 2020

rebased on top of the current master branch

@hhugo hhugo changed the title {Q,Z},of_string accept underscore {Q,Z},of_string accept underscores Sep 21, 2020
@hhugo
Copy link
Contributor Author

hhugo commented Sep 22, 2020

I've done some cleanup and added more tests.

xavierleroy
xavierleroy reviewed Oct 10, 2020
View reviewed changes
Copy link
Contributor

@xavierleroy xavierleroy left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the tests! A minor style suggestion below.

caml_z.c Outdated Show resolved Hide resolved
@antoinemine
Copy link
Collaborator

Hi !
I agree with Xavier's remark (I'm especially not fond of the j-- before the continue).
Also, we should free digits in case of caml_invalid_argument to avoid memory leaks.

@hhugo
Copy link
Contributor Author

hhugo commented Oct 11, 2020 •
edited

  • I've applied your suggestion
  • freed digits before caml_invalid_arguement
  • cleaned up the implementation further

@hhugo
Copy link
Contributor Author

hhugo commented Oct 13, 2020

hold on merging this one, I'm investigating some potential bug.

@hhugo
Copy link
Contributor Author

hhugo commented Oct 13, 2020

False alert. ready to merge.

xavierleroy
xavierleroy approved these changes Oct 13, 2020
View reviewed changes
Copy link
Contributor

@xavierleroy xavierleroy left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks.

@xavierleroy xavierleroy merged commit a28c466 into ocaml:master Oct 13, 2020
@hhugo hhugo deleted the underscore branch October 13, 2020 16:45
@hhugo hhugo mentioned this pull request Oct 13, 2020
Merged
@skirpichev skirpichev mentioned this pull request May 31, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xavierleroy xavierleroy xavierleroy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make Z.of_string and Q.of_string support underscores
3 participants
@hhugo @antoinemine @xavierleroy