Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[mono][wasm] Fix function signature mismatch in m2n invoke #101106
[mono][wasm] Fix function signature mismatch in m2n invoke #101106
Changes from 15 commits
affbf56
f49a1c4
32358c0
d48ecb1
ca68d37
9159c02
ca36d4c
7634d53
022420a
5a3a58d
6e5beba
1bfd0f0
acd32e4
186a15f
899acbe
0b65450
9ba5a4a
46df3ec
b3d533f
a85f7c6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why this needs to be wasm-specific, shouldn't we do the same logic everywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It broke
iOS/tvOS
tests that's why I applied the logic only for wasmThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm that sounds concerning, I don't see why it should break iOS/tvOS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what error looked like (from https://dev.azure.com/dnceng-public/public/_build/results?buildId=652242&view=logs&j=88e1dda0-08c6-5d85-6ee4-8e42aeff31b6&t=b4bb5080-7a46-5114-c100-ade75b904ada)
My guess is that for iOS/tvOS in other places it still expects the previous logic. And I'm not sure that for other platforms there is a need to apply the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then we need to find these places, the logic should be consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure of the exact constraints for this name mapping algorithm but could we just replace all chars outside of a-z/0-9 with underscore? that would allow us to not allocate in the native side
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed when mapping two assemblies to the same name.
For example having an app project that references a library project named in a way that maps to the same name, but is different before fixup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah so e.g.
HelloÖProject.DoSomething()
andHelloÜProject.DoSomething()
would both map toHello_Project_DoSomething
, makes sense.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure whether it'd be strictly better but we could also collect the special chars and append them at the end or return them separately. that'd have the benefit of not needing to allocate on the native side when the string doesn't have special chars
@lambdageek for thoughts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how adding the characters in a suffix would make a difference.
On the native side I guess it would be nice to have a function that precomputes the required space for the entire mangled name and just does a single allocation. (ie: get rid of the
g_strdup_printf
with the prefix and the name together with the mangling loop - and just make a helper that that does a single allocation for the final mangled name)