Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Label implements IEquatable<Label> #35673

Merged
merged 5 commits into from
Apr 6, 2019
Merged

Label implements IEquatable<Label> #35673

merged 5 commits into from
Apr 6, 2019

Conversation

Foxtrek64
Copy link

@Foxtrek64 Foxtrek64 commented Mar 1, 2019 •
edited
Loading

Adds an implementation of IEquatable<Label> to System.Reflection.Emit.Label.

Fixes dotnet/corefx#32959
Relies on dotnet/corecrl/pull/22938

@Foxtrek64 Foxtrek64 mentioned this pull request Mar 1, 2019
Merged
@danmoseley
Copy link
Member

Thanks @Foxtrek64 . Can you please add a simple test, that fails before your change and passes after? Something that calls Equals through IEquatable<Label>

@danmoseley
Copy link
Member

The tests are apparently in src\System.Reflection.Emit.ILGeneration\tests\Label\LabelEquals.cs

@Foxtrek64
Copy link
Author

I was unable to run the unit tests through VS (kept throwing System.BadImageFormatException: Bad IL Format) even after a successful build of both CoreCLR and CoreFx.

So I relied on msbuild to do it for me. It seems happy enough.

PS m:\corefx\src\System.Reflection.Emit.ILGeneration\tests> dotnet msbuild /t:BuildAndTest
Microsoft (R) Build Engine version 16.0.385-preview+g966cdf2ac6 for .NET Core
Copyright (C) Microsoft Corporation. All rights reserved.

  System.Reflection.Emit.ILGeneration.Tests -> m:\corefx\artifacts\bin\System.Reflection.Emit.ILGeneration.Tests\netcore
app-Debug\System.Reflection.Emit.ILGeneration.Tests.dll
  ----- start 23:12:02.02 ===============  To repro directly: =====================================================
  pushd m:\corefx\artifacts\bin\tests\System.Reflection.Emit.ILGeneration.Tests\netcoreapp-Windows_NT-Debug-x64\
  m:\corefx\artifacts\bin\testhost\netcoreapp-Windows_NT-Debug-x64\dotnet.exe xunit.console.dll System.Reflection.Emit.I
LGeneration.Tests.dll -xml testResults.xml -nologo -notrait category=nonnetcoreapptests -notrait category=nonwindowstest
s -notrait category=failing -notrait category=OuterLoop
  popd
ne)
    Discovered:  System.Reflection.Emit.ILGeneration.Tests (found 128 test cases)
    Starting:    System.Reflection.Emit.ILGeneration.Tests (parallel test collections = on, max threads = 8)
  0
  emitWriteLine
  False
    Finished:    System.Reflection.Emit.ILGeneration.Tests
  === TEST EXECUTION SUMMARY ===
     System.Reflection.Emit.ILGeneration.Tests  Total: 222, Errors: 0, Failed: 0, Skipped: 0, Time: 18.655s
  ----- end 23:14:29.01 ----- exit code 0 ----------------------------------------------------------

@karelz
Copy link
Member

karelz commented Mar 18, 2019

@GrabYourPitchforks @steveharter can you please review it?
cc @joshfree

@steveharter
Copy link
Member

@Foxtrek64 can you rebase so CI can see the changes made to the CLR? Thanks.

D:\a\1\s\.packages\microsoft.dotnet.apicompat\1.0.0-beta.19128.2\build\Microsoft.DotNet.ApiCompat.targets(72,5): error : CannotRemoveBaseTypeOrInterface : Type 'System.Reflection.Emit.Label' does not implement interface 'System.IEquatable<System.Reflection.Emit.Label>' in the implementation but it does in the contract. [D:\a\1\s\src\System.Reflection.Emit.ILGeneration\src\System.Reflection.Emit.ILGeneration.csproj]
D:\a\1\s\.packages\microsoft.dotnet.apicompat\1.0.0-beta.19128.2\build\Microsoft.DotNet.ApiCompat.targets(85,5): error : ApiCompat failed for 'D:\a\1\s\artifacts\bin\System.Reflection.Emit.ILGeneration\uap10.0.16299-Debug\System.Reflection.Emit.ILGeneration.dll' [D:\a\1\s\src\System.Reflection.Emit.ILGeneration\src\System.Reflection.Emit.ILGeneration.csproj]

@Foxtrek64 Foxtrek64 force-pushed the feature/labelIEquatable branch 2 times, most recently from 25e4c8b to d836988 Compare March 24, 2019 02:47
@Foxtrek64
Copy link
Author

I've done a couple rebases (the second one just to make sure I did it right) and the packing test is still failing. If you guys have any ideas on something I may have missed, I'm all ears.

@steveharter
Copy link
Member

I see now the packaging error was just for uap.

@joperezr @ericstj do we need to add to an ApiCompatBaseline.uapaot.txt file to remove the packaging error?

@karelz
Copy link
Member

karelz commented Apr 1, 2019

@ericstj @safern can you please help with the question above?

@karelz karelz added this to the 3.0 milestone Apr 1, 2019
@ericstj
Copy link
Member

ericstj commented Apr 2, 2019 

D:\a\1\s\.packages\microsoft.dotnet.apicompat\1.0.0-beta.19170.2\build\Microsoft.DotNet.ApiCompat.targets(72,5): error : CannotRemoveBaseTypeOrInterface : Type 'System.Reflection.Emit.Label' does not implement interface 'System.IEquatable<System.Reflection.Emit.Label>' in the implementation but it does in the contract. [D:\a\1\s\src\System.Reflection.Emit.ILGeneration\src\System.Reflection.Emit.ILGeneration.csproj]
D:\a\1\s\.packages\microsoft.dotnet.apicompat\1.0.0-beta.19170.2\build\Microsoft.DotNet.ApiCompat.targets(85,5): error : ApiCompat failed for 'D:\a\1\s\artifacts\bin\System.Reflection.Emit.ILGeneration\uap10.0.16299-Debug\System.Reflection.Emit.ILGeneration.dll' [D:\a\1\s\src\System.Reflection.Emit.ILGeneration\src\System.Reflection.Emit.ILGeneration.csproj]

Note the uap10.0.16299-Debug in this path. You've modified the reference assembly which applies to the last version of UAP (and older versions of .NETCoreApp...). I think you need to put the change in https://github.com/dotnet/corefx/blob/d83698810f608688f103951a1d37ad749c8a3a5f/src/System.Reflection.Emit.ILGeneration/ref/System.Reflection.Emit.ILGeneration.netcore.cs instead.

@Foxtrek64
Copy link
Author

That file doesn't have a Label declaration. Should I add one, perhaps cloning the code from this declaration?

corefx/src/System.Reflection.Emit.ILGeneration/ref/System.Reflection.Emit.ILGeneration.cs

Lines 59 to 67 in d836988

public partial struct Label : IEquatable<System.Reflection.Emit.Label>
{
private int _dummyPrimitive;
public override bool Equals(object obj) { throw null; }
public bool Equals(System.Reflection.Emit.Label obj) { throw null; }
public override int GetHashCode() { throw null; }
public static bool operator ==(System.Reflection.Emit.Label a, System.Reflection.Emit.Label b) { throw null; }
public static bool operator !=(System.Reflection.Emit.Label a, System.Reflection.Emit.Label b) { throw null; }
}

@ericstj
Copy link
Member

ericstj commented Apr 2, 2019

It’s an additional file, you cannot redefine members defined in another file. Just make sure the class is partial and add the interface implementation in the additional file. It’s just a handy way of doing ifdef’s if they can be represented using partial classes.

@Foxtrek64
Copy link
Author

@ericstj Eric, could you clarify your last post? I'm not entirely certain what you mean.
"Just make sure the class is partial...." What class?
"...in the additional file." What additional file?

@ericstj
Copy link
Member

ericstj commented Apr 2, 2019

What class?

Apologies, in this case its a struct: Label. It is partial already:

corefx/src/System.Reflection.Emit.ILGeneration/ref/System.Reflection.Emit.ILGeneration.cs

Line 59 in aa3865a

public partial struct Label

What additional file?

The file I suggested that you change. https://github.com/dotnet/corefx/blob/d83698810f608688f103951a1d37ad749c8a3a5f/src/System.Reflection.Emit.ILGeneration/ref/System.Reflection.Emit.ILGeneration.netcore.cs

In other words, don't modify System.Reflection.Emit.ILGeneration.cs, literally only add the following to System.Reflection.Emit.ILGeneration.netcore.cs:

public partial struct Label : IEquatable<System.Reflection.Emit.Label>
{ }

ericstj
ericstj approved these changes Apr 2, 2019
View reviewed changes
Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

LGTM

@stephentoub
Copy link
Member

Thanks.

@stephentoub stephentoub merged commit b561cd4 into dotnet:master Apr 6, 2019
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@Foxtrek64 @stephentoub
Label implements IEquatable<Label> (dotnet/corefx#35673)
6e131e9 
* Label implements IEquatable<Label>

* Add Unit Tests for IEquatable<Label>

* Actually track LabelEquals.cs this time

* Move Label IEquatable declaration to ILGeneration.netcore

* Merge IEquatable unit tests with existing equality tests


Commit migrated from dotnet/corefx@b561cd4
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@stephentoub stephentoub stephentoub left review comments

@ericstj ericstj ericstj approved these changes

@steveharter steveharter steveharter approved these changes

@danmoseley danmoseley danmoseley approved these changes

Assignees

@GrabYourPitchforks GrabYourPitchforks

@steveharter steveharter

@Foxtrek64 Foxtrek64

Labels
area-System.Reflection.Emit
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.
7 participants
@Foxtrek64 @danmoseley @karelz @steveharter @ericstj @stephentoub @GrabYourPitchforks