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

Correct signature of memmove #34165

Merged
merged 1 commit into from
Dec 23, 2019
Merged

Correct signature of memmove #34165

merged 1 commit into from
Dec 23, 2019

Conversation

Keno
Copy link
Member

@Keno Keno commented Dec 20, 2019

wasm was complaining about the incorrect Cvoid rather than Ptr{Void}
in the :memmove calls in ryu, but while we're here also change the
third argument to Csize_t, since that's what the standard says it is.

wasm was complaining about the incorrect Cvoid rather than Ptr{Void}
in the :memmove calls in ryu, but while we're here also change the
third argument to Csize_t, since that's what the standard says it is.
@vtjnash
Copy link
Member

vtjnash commented Dec 20, 2019

Wasn't the decision in #31464 to commit to detecting this pair of functions as a special case in the backend and make this signature mismatch valid?

@Keno
Copy link
Member Author

Keno commented Dec 21, 2019

Wasn't the decision in #31464 to commit to detecting this pair of functions as a special case in the backend and make this signature mismatch valid?

We certainly decided that for memcpy which we explicitly intercept in ccall.cpp. I'm fine with extending that to memmove also, though of course that would mean adding an appropriate special case in ccall.cpp.

@JeffBezanson JeffBezanson merged commit 8bd2eb1 into master Dec 23, 2019
@JeffBezanson JeffBezanson deleted the kf/memmovesig branch December 23, 2019 01:46
KristofferC pushed a commit that referenced this pull request Apr 11, 2020
wasm was complaining about the incorrect Cvoid rather than Ptr{Void}
in the :memmove calls in ryu, but while we're here also change the
third argument to Csize_t, since that's what the standard says it is.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants