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

Add CreateICW to Interop Gdi32 #2881

Merged
merged 1 commit into from Feb 24, 2020
Merged

Add CreateICW to Interop Gdi32 #2881

merged 1 commit into from Feb 24, 2020

Conversation

gpetrou
Copy link
Contributor

@gpetrou gpetrou commented Feb 19, 2020

Proposed changes

  • Add CreateICW to Interop Gdi32.
  • Remove CreateIC from UnsafeNativeMethods and replace its usages.
Microsoft Reviewers: Open in CodeFlow

@gpetrou gpetrou requested a review from a team as a code owner February 19, 2020 07:02
@ghost ghost assigned gpetrou Feb 19, 2020
@codecov
Copy link

codecov bot commented Feb 19, 2020

Codecov Report

Merging #2881 into master will decrease coverage by 0.00832%.
The diff coverage is 100%.

@@                 Coverage Diff                 @@
##              master       #2881         +/-   ##
===================================================
- Coverage   59.83718%   59.82885%   -0.00833%     
===================================================
  Files           1241        1241                 
  Lines         431775      431775                 
  Branches       38811       38811                 
===================================================
- Hits          258362      258326         -36     
- Misses        168059      168098         +39     
+ Partials        5354        5351          -3
Flag Coverage Δ
#Debug 59.82885% <100%> (-0.00833%) ⬇️
#production 32.35637% <100%> (-0.01419%) ⬇️
#test 98.97284% <ø> (ø) ⬆️

Copy link
Member

@RussKie RussKie 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 👍

But I propose we take it a bit further by reducing unnecessary allocations and interop marshaling overheads by using char* instead of strings.
It is a good time to add unit tests to very the behaviour before and after the change as well.

@gpetrou
Copy link
Contributor Author

gpetrou commented Feb 19, 2020

I thought that when we just pass parameters it is OK to use string? Also, looking at the code coverage seems like the RightMargin property lines are already covered?

@RussKie
Copy link
Member

RussKie commented Feb 19, 2020

I thought that when we just pass parameters it is OK to use string?

TL;DR: it works, but it is not efficient.

string isn't blittable, and that means another string will need to be allocated in the unmanaged heap. By making the signature char* we pin the managed string and let the unmanaged code to access it directly.
This is the overhead I mentioned earlier.

RightMargin property lines are already covered?

There is a single test that asserts RightMargin == 0.
We need tests that set values with and without Handle created, and assert it works correctly.

@gpetrou
Copy link
Contributor Author

gpetrou commented Feb 20, 2020

In RichTextBoxTests I see many calls to RightMargin, not one. Am I missing something here?

@RussKie
Copy link
Member

RussKie commented Feb 21, 2020

In RichTextBoxTests I see many calls to RightMargin, not one. Am I missing something here?

You're right! I was looking at an outdated branch (I have several working repos).
So we should be in a good position to refactor it then.

@RussKie RussKie merged commit fa6e4b0 into dotnet:master Feb 24, 2020
@ghost ghost added this to the 5.0 milestone Feb 24, 2020
@RussKie RussKie added the enhancement Product code improvement that does NOT require public API changes/additions label Feb 24, 2020
@gpetrou gpetrou deleted the CreateICW branch February 25, 2020 07:52
M-Lipin pushed a commit to M-Lipin/winforms that referenced this pull request Mar 23, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Feb 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants