Cleanup EDITSTREAM

[hughbe]

Proposed Changes

• Cleanup EDITSTREAM

Microsoft Reviewers: Open in CodeFlow

[RussKie]

MC, and (I suspect) test failures

[RussKie]

Have you tested it in any way?

[hughbe]

hmm there may be different padding on 64 bit vs. 32 bit structs. @weltkante if you have any immediate things you notice

[weltkante]

hmm there may be different padding on 64 bit vs. 32 bit structs. @weltkante if you have any immediate things you notice

Yes, you are adding 4 byte undesired padding in 64bit between these fields. You can solve this without making two structs by forcing alignment to 4 byte instead of keeping it at native size. So adding a [StructLayout(LayoutKind.Sequential, Pack = 4)] should do the trick.

[weltkante]

For the record, in the header file the place where it deviates from standard packing rules is by declaring

#ifdef _WIN32
#include <pshpack4.h>
#elif !defined(RC_INVOKED)
#pragma pack(4)
#endif

This is which forces Pack=4 in C# terms for all structs in this header file. Only relevant if the struct contains pointers or doubles (i.e. anything larger than 4 byte)

It's unfortunate that the headers have these sprinkled around instead of directly on the declarations like C# enforces, you always have to search for them if you want to make sure you are using the right packing rules.

[RussKie]

Keeping it as "unresolved" for the ease of future searching

[JeremyKuhne]

When I'm writing unmanaged struct wrappers I'll sometimes write unit tests that check sizeof-- particularly for less common or complicated structures. I manually check the sizeof in a C++ dll and hard code the values, but that could also be done in code by having an export that allows you to query.

I'd consider:

1. Writing tests that check the size
2. Testing interop in both x86 and x64
3. Using a cross-compiled native dll to expose the C sizeof for structs (in addition to other native helpers for testing)

[RussKie]

• Writing tests that check the size

TBD.

• Testing interop in both x86 and x64

I have raised #2879 for CI enhancements. [EDIT] Merged

• Using a cross-compiled native dll to expose the C sizeof for structs (in addition to other native helpers for testing)

@hughbe you were working on #1932. Perhaps it can be leveraged for this purpose.

[hughbe]

@hughbe you were working on #1932. Perhaps it can be leveraged for this purpose.

This is not going to be merged for sometime as it is waiting on upstream arcade changes.

I have written a bunch of tests to ensure near 100% coverage as well as verifying the size on x86 and x64

[codecov]

Codecov Report

Merging #2816 into master will increase coverage by 0.09665%.
The diff coverage is 97.61905%.

@@                 Coverage Diff                 @@
##              master       #2816         +/-   ##
===================================================
+ Coverage   60.27793%   60.37459%   +0.09666%     
===================================================
  Files           1249        1249                 
  Lines         433633      434552        +919     
  Branches       38850       38849          -1     
===================================================
+ Hits          261385      262359        +974     
+ Misses        166879      166822         -57     
- Partials        5369        5371          +2

Flag	Coverage Δ
#Debug	60.37459% <97.61905%> (+0.09665%)	⬆️
#production	32.48554% <97.61905%> (+0.02156%)	⬆️
#test	98.96922% <ø> (-0.00617%)	⬇️

[RussKie]

What happened to EDITSTREAM clean up? Why do we have only test changes?

[hughbe]

I was testing to see whether my changes regressed any tests. Turns out we may have a bug. I wrote the tests against full .NET Framework but they fail on .NET Core - it looks like SelectedRtf may be broken

    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_GetWithPlainTextWithHandle_ReturnsExpected [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1309,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_GetWithPlainTextWithHandle_ReturnsExpected()
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_GetWithPlainText_ReturnsExpected [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1282,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_GetWithPlainText_ReturnsExpected()
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_Set_GetReturnsExpected(nullOrEmpty: null) [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1336,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_Set_GetReturnsExpected(String nullOrEmpty)
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_Set_GetReturnsExpected(nullOrEmpty: "") [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1336,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_Set_GetReturnsExpected(String nullOrEmpty)
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_GetWithHandle_ReturnsExpected [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1257,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_GetWithHandle_ReturnsExpected()
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithHandle_GetReturnsExpected(nullOrEmpty: null) [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1365,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithHandle_GetReturnsExpected(String nullOrEmpty)
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithHandle_GetReturnsExpected(nullOrEmpty: "") [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1365,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithHandle_GetReturnsExpected(String nullOrEmpty)
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithRtfWithHandle_GetReturnsExpected(nullOrEmpty: null) [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1428,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithRtfWithHandle_GetReturnsExpected(String nullOrEmpty)
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithRtfWithHandle_GetReturnsExpected(nullOrEmpty: "") [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1428,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithRtfWithHandle_GetReturnsExpected(String nullOrEmpty)
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithRtf_GetReturnsExpected(nullOrEmpty: null) [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1396,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithRtf_GetReturnsExpected(String nullOrEmpty)
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithRtf_GetReturnsExpected(nullOrEmpty: "") [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1396,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_SetWithRtf_GetReturnsExpected(String nullOrEmpty)
    System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_Get_ReturnsExpected [FAIL]
      Assert.StartsWith() Failure:
      Expected: {\rtf
      Actual:   
      Stack Trace:
        /_/src/System.Windows.Forms/tests/UnitTests/System/Windows/Forms/RichTextBoxTests.cs(1234,0): at System.Windows.Forms.Tests.RichTextBoxTests.RichTextBox_SelectedRtf_Get_ReturnsExpected()

[weltkante]

I can't quite follow, when I look at the SelectedRtf tests they seem wrong to me and they also fail on Desktop Framework.

In short, if there is no Text or SelectedText then SelectedRtf is empty. That behavior also happens on Desktop for me and IMHO its reasonable behavior. Note that there exist different versions of the RTF control and maybe you're running your Desktop tests against an older version which behaved differently. .NET Core only supports using the newest version.

[hughbe]

Note that there exist different versions of the RTF control and maybe you're running your Desktop tests against an older version which behaved differently. .NET Core only supports using the newest version

Yeah seems like this was the problem. Cheers!

[RussKie]

Docs: dotnet/docs#17085

[RussKie]

@weltkante any more concerns?