Skip to content

Add QCalls to the Runtime#19945

Merged
thaystg merged 1 commit intomono:masterfrom
monojenkins:sync-pr-37670-from-runtime
Jun 19, 2020
Merged

Add QCalls to the Runtime#19945
thaystg merged 1 commit intomono:masterfrom
monojenkins:sync-pr-37670-from-runtime

Conversation

@monojenkins
Copy link
Copy Markdown
Contributor

@monojenkins monojenkins commented Jun 9, 2020

!! This PR is a copy of dotnet/runtime#37670, please do not edit or review it in this repo !!
Do not automatically approve this PR:

* Consider how the changes affect configurations in this repo,
* Check effects on files that are not mirrored,
* Identify test cases that may be needed in this repo.

!! Merge the PR only after the original PR is merged !!



- Implementing QCalls on mono

  • Use QCalls on ICU Shim implementation

Fixes #dotnet/runtime#36449

Comment thread mono/metadata/qcall-eventpipe.c Outdated
Copy link
Copy Markdown
Contributor

@jaykrell jaykrell Jun 10, 2020

Choose a reason for hiding this comment

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

should be
extern char const * const name [] = { ... }
and remove the casts. constant pointers to constant characters.
What you have is mutable pointers to constant void.
constant pointers are generally more efficient and more secure and less error-prone.

Comment thread mono/metadata/native-library.c Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Must this be strcmp?
Not an integer or enum, or a string with a length?
strcmp is slow.

Comment thread mono/metadata/native-library-qcall.c Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: one space after return instead of two

Comment thread mono/metadata/native-library-qcall.c Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please figure out how to use qsort or std::sort here, instead of inlining it.
If you need inlining for perf, take an existing qsort and change it to a slightly ugly but fair enough macro. It's been done, it isn't hard, and it can be argued for perf.

Comment thread mono/metadata/native-library-qcall.c Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: space between function name and the opening paren to call it.

Comment thread mono/metadata/native-library-qcall.c Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: spaces around binary operators: low = mid + 1
but really, delete all this code anyway and use qsort or std::sort or a macro.

@monojenkins monojenkins force-pushed the sync-pr-37670-from-runtime branch 19 times, most recently from e131798 to 043c2bf Compare June 17, 2020 12:23
@thaystg
Copy link
Copy Markdown
Contributor

thaystg commented Jun 18, 2020

@monojenkins build

@monojenkins monojenkins force-pushed the sync-pr-37670-from-runtime branch from 043c2bf to e1eb9ee Compare June 18, 2020 23:13
@thaystg thaystg marked this pull request as ready for review June 19, 2020 01:47
@monojenkins monojenkins force-pushed the sync-pr-37670-from-runtime branch from e1eb9ee to 8a82b4f Compare June 19, 2020 01:50
- Implementing QCalls on mono
- Use QCalls on ICU Shim implementation

Fixes dotnet/runtime#36449
@monojenkins monojenkins force-pushed the sync-pr-37670-from-runtime branch from 8a82b4f to 63cf4f0 Compare June 19, 2020 03:39
@thaystg
Copy link
Copy Markdown
Contributor

thaystg commented Jun 19, 2020

@monojenkins build failed

@thaystg thaystg merged commit 99794ef into mono:master Jun 19, 2020
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.

3 participants