Remove StructCache by sharwell · Pull Request #818 · dotnet/winforms

sharwell · 2019-04-17T02:46:36Z

Follow-up to #791. ~~I will rebase this change after the change from @hughbe is merged.~~

codecov · 2019-04-17T02:58:17Z

Codecov Report

Merging #818 into master will increase coverage by 0.02015%.
The diff coverage is 0%.

@@                Coverage Diff                @@
##              master      #818         +/-   ##
=================================================
+ Coverage   28.05285%   28.073%   +0.02015%     
=================================================
  Files           1065      1065                 
  Lines         292530    292402        -128     
  Branches       38519     38508         -11     
=================================================
+ Hits           82063     82086         +23     
+ Misses        206407    206253        -154     
- Partials        4060      4063          +3

Flag	Coverage Δ
#Debug	`28.073% <0%> (+0.02015%)`	⬆️
#production	`18.38533% <0%> (+0.01809%)`	⬆️
#test	`98.66795% <ø> (ø)`	⬆️

weltkante

Please check if ELEMDESC is defined correctly

weltkante · 2019-04-17T07:38:13Z

src/Common/src/NativeMethods.cs

Is it correct making this a pointer? ELEMDESC embeds TYPEDESC by value, doing it by reference will lose the VARTYPE member and have the following PARAMDESC at the wrong alignment.

(That probably means the old struct was defined wrong too? If so this might need an additional code review of the callers to check if they need to be updated if they were previously coded/tested with wrong assumptions.)

It's not clear to me why it's defined this way and also not clear why it's not using System.Runtime.InteropServices.ComTypes.ELEMDESC. I would rather have tests in place before making functional changes to the signatures.

@sharwell you may be able to track down the blame for this in the old source control system and followup with the author about why these lines are the way that they are

I've created follow up issue #896 to track the wrong definition

src/Common/src/NativeMethods.cs

hughbe · 2019-04-17T09:12:54Z

src/Common/src/NativeMethods.cs

Don't think these are necessary

I agree, but I would prefer to have tests in place to verify before dropping them since I'm not 100%.

They aren't necessary. I don't know if the marshaller ignores the redundancy here or if that makes it do extra work. @jkoritzinsky, are these just ignored or will they impact blittability logic?

The marshalling system ignores the redundancy here. Having these doesn't impact blittability.

zsd4yr · 2019-04-19T16:31:00Z

#791 is merged

/cc @JeremyKuhne

sharwell · 2019-04-19T18:23:19Z

I rebased this and addressed the resulting merge conflicts, but have not addressed the other feedback items yet.

danmoseley · 2019-04-21T06:33:33Z

cc @JeremyKuhne

JeremyKuhne · 2019-04-23T16:48:56Z

src/Common/src/NativeMethods.cs

-            public    /*NativeMethods.tagELEMDESC*/ IntPtr lprgelemdescParam = IntPtr.Zero;
+            public    /*NativeMethods.tagELEMDESC*/ IntPtr lprgelemdescParam;

            // cpb, Microsoft, the EE chokes on Enums in structs


This isn't a true statement.

JeremyKuhne · 2019-04-23T17:04:40Z

src/Common/src/NativeMethods.cs

-            public IntPtr lprgscode = IntPtr.Zero;
+            public IntPtr lprgscode;

            // This is marked as NATIVE_TYPE_PTR,


All of these comments (before the next member) should be removed- they're not correct.

JeremyKuhne · 2019-04-23T17:11:03Z

src/Common/src/NativeMethods.cs

-        public  sealed class  tagTYPEATTR {
+        public  struct  tagTYPEATTR {
            public Guid guid;
            [MarshalAs(UnmanagedType.U4)]


All of these can be removed. It doesn't matter if the field is int or uint when matching up DWORD. That said, prefer to the actual signed/unsigned type to match (e.g. uint for DWORD).

JeremyKuhne · 2019-04-23T17:11:28Z

src/Common/src/NativeMethods.cs

@@ -5239,53 +5233,53 @@ public interface IProvideClassInfo
        }

        [StructLayout(LayoutKind.Sequential)]


The attribute isn't needed- this is the default for structs.

JeremyKuhne

Awesome that we're getting rid of these classes!

We should look at yanking all of these and using the ones defined in System.Runtime.InteropServices.ComTypes. They're correctly defined as structs so there won't be allocation issues like they're trying to avoid here.

Don't hesitate to ping me if you're finding any difficulties using the .NET types here.

sharwell · 2019-04-23T21:55:36Z

We should look at yanking all of these and using the ones defined in System.Runtime.InteropServices.ComTypes

I think this is my preference, but they aren't the drop in replacements I was hoping for. I'll follow up with details next week and we can figure out the best plan for moving forward.

zsd4yr · 2019-04-24T16:21:31Z

We should look at yanking all of these and using the ones defined in System.Runtime.InteropServices.ComTypes

I think this is my preference, but they aren't the drop in replacements I was hoping for. I'll follow up with details next week and we can figure out the best plan for moving forward.

I think this is the best idea moving forward. @sharwell I would love to see what you come up with next week 😄

For now, I am content to take this change if you are.

danmoseley · 2019-04-27T20:39:41Z

@zsd4yr seems this can be merged?

danmoseley · 2019-04-27T21:51:26Z

I'm going to merge this based on @zsd4yr approval above.

ELEMDESC doesn't contain a pointer. This fixes a regression introduced in dotnet#818. Fixes dotnet#896

ELEMDESC doesn't contain a pointer. This fixes a regression introduced in #818. Fixes #896

ELEMDESC doesn't contain a pointer. This fixes a regression introduced in dotnet#818. Fixes dotnet#896

ELEMDESC doesn't contain a pointer. This fixes a regression introduced in #818. Fixes #896

ELEMDESC doesn't contain a pointer. This fixes a regression introduced in #818. Fixes #896 (cherry picked from commit 52be7e5)

sharwell requested a review from a team as a code owner April 17, 2019 02:46

weltkante suggested changes Apr 17, 2019

View reviewed changes

hughbe reviewed Apr 17, 2019

View reviewed changes

sharwell force-pushed the struct-cache branch from 5e2f47f to e6ef896 Compare April 19, 2019 18:22

sharwell added 8 commits April 19, 2019 16:08

Add UnsafeNativeMethods.PtrToRef<T>

2077fcb

Convert tagTYPEATTR to a value type

008a242

Convert tagTYPEDESC to a value type

b0187d3

Convert tagFUNCDESC to a value type

d576ffa

Convert tagVARDESC to a value type

82fc206

Convert tagELEMDESC to a value type

879c760

Remove unnecessary StructCache

0187bb3

Remove duplicate types

6b68815

sharwell force-pushed the struct-cache branch from 9f9cee7 to 6b68815 Compare April 19, 2019 21:09

sharwell mentioned this pull request Apr 19, 2019

Add GetLParam<T> and GetLParamRef<T> #835

Closed

JeremyKuhne reviewed Apr 23, 2019

View reviewed changes

JeremyKuhne requested changes Apr 23, 2019

View reviewed changes

zsd4yr approved these changes Apr 24, 2019

View reviewed changes

danmoseley merged commit 2dfd866 into dotnet:master Apr 27, 2019

weltkante mentioned this pull request Apr 28, 2019

Fix ELEMDESC native interop definition #896

Closed

sharwell mentioned this pull request Apr 28, 2019

Use Unsafe.AsRef to implement UnsafeNativeMethods.PtrToRef #898

Merged

JeremyKuhne added a commit to JeremyKuhne/winforms that referenced this pull request Jul 1, 2019

Fix COM interop ELEMDESC

29457fa

ELEMDESC doesn't contain a pointer. This fixes a regression introduced in dotnet#818. Fixes dotnet#896

JeremyKuhne mentioned this pull request Jul 1, 2019

Fix COM interop ELEMDESC #1254

Merged

wtgodbe pushed a commit that referenced this pull request Jul 2, 2019

Fix COM interop ELEMDESC (#1254)

2b3a4a4

ELEMDESC doesn't contain a pointer. This fixes a regression introduced in #818. Fixes #896

wtgodbe pushed a commit to wtgodbe/winforms that referenced this pull request Jul 2, 2019

Fix COM interop ELEMDESC (dotnet#1254)

f9cdc6c

ELEMDESC doesn't contain a pointer. This fixes a regression introduced in dotnet#818. Fixes dotnet#896

wtgodbe mentioned this pull request Jul 2, 2019

Fix COM interop ELEMDESC (#1254) #1266

Merged

AdamYoblick pushed a commit that referenced this pull request Jul 2, 2019

Fix COM interop ELEMDESC (#1254) (#1266)

52be7e5

ELEMDESC doesn't contain a pointer. This fixes a regression introduced in #818. Fixes #896

RussKie pushed a commit that referenced this pull request Jul 3, 2019

Fix COM interop ELEMDESC (#1254) (#1266)

73c150f

ELEMDESC doesn't contain a pointer. This fixes a regression introduced in #818. Fixes #896 (cherry picked from commit 52be7e5)

ghost locked as resolved and limited conversation to collaborators Feb 6, 2022

		@@ -5239,53 +5233,53 @@ public interface IProvideClassInfo
		}

		[StructLayout(LayoutKind.Sequential)]

Conversation

sharwell commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

weltkante left a comment

Choose a reason for hiding this comment

Uh oh!

weltkante Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zsd4yr commented Apr 19, 2019

Uh oh!

sharwell commented Apr 19, 2019

Uh oh!

danmoseley commented Apr 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JeremyKuhne left a comment

Choose a reason for hiding this comment

Uh oh!

sharwell commented Apr 23, 2019

Uh oh!

zsd4yr commented Apr 24, 2019

Uh oh!

danmoseley commented Apr 27, 2019

Uh oh!

danmoseley commented Apr 27, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

sharwell commented Apr 17, 2019 •

edited

Loading

codecov bot commented Apr 17, 2019 •

edited

Loading

weltkante Apr 17, 2019 •

edited

Loading