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

Mark the String.equal external as not-allocating C. #2159

Merged
merged 2 commits into from Nov 21, 2018

Conversation

Projects
None yet
3 participants
@ppedrot
Copy link
Contributor

commented Nov 21, 2018

It is indeed a mere loop that does not allocate. This function was probably forgotten because its code was hidden deep in the module.

Fixes Mantis#7874.

Mark the String.equal external as not-allocating C.
It is indeed a mere loop that does not allocate. This function was probably
forgotten because its code was hidden deep in the module.
@nojb

nojb approved these changes Nov 21, 2018

Copy link
Contributor

left a comment

LGTM. Did you check if there was any other forgotten noalloc external in string.ml or bytes.ml?

@ppedrot

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

I did check in String but not in Bytes.

@ppedrot

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

Bytes has the same issue, I pushed a similar patch for that file.

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

It looks like the annotation usually appears in both the .ml and .mli files.
(and if you do that change, it also applies to labelled versions of the module.)

@ppedrot

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

Really? Isn't noalloc supposed to be tied to an external command? The mli interface doesn't export this.

@xclerc

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

(Sorry for the noise, I thought equal was an external in the .mli file.)

@ppedrot

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

Does this deserve a change? It's hardly user-visible.

@nojb nojb merged commit abbc5f3 into ocaml:trunk Nov 21, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@nojb

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

Thanks! I added a Changes entry in ea6ce52.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.