Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Dictionary exposes internal type comparer#16151

Merged
jkotas merged 1 commit into
dotnet:masterfrom
MarcoRossignoli:corefx_26033
Feb 3, 2018
Merged

Dictionary exposes internal type comparer#16151
jkotas merged 1 commit into
dotnet:masterfrom
MarcoRossignoli:corefx_26033

Conversation

@MarcoRossignoli
Copy link
Copy Markdown
Member

@MarcoRossignoli MarcoRossignoli commented Feb 1, 2018

Fixes https://github.com/dotnet/corefx/issues/26033

This is my first pull on this repo(maybe i'm in the wrong place). I don't know if there is a "guide" to do "cross repo" test for core*.
If i want to add a test on corefx with this "merge", do i have to wait and clean/rebuild?

cc: @jkotas

@danmoseley
Copy link
Copy Markdown
Member

@maryamariyan could you please guide @MarcoRossignoli through writing a matching CoreFX test, and help him verify it against his private CoreCLR change? Since you have been doing this recnetly.

@maryamariyan
Copy link
Copy Markdown

maryamariyan commented Feb 1, 2018

@danmosemsft sure.

@MarcoRossignoli here's what you need to do:

  • Build coreclr locally in release mode. You can skip tests to complete the build faster.
C:\git\coreclr> git clean -fdx 
C:\git\coreclr> build -release -skiptests 
  • Follow instructions to run corefx tests using local coreclr bits here
C:\git\corefx> git clean -fdx
C:\git\corefx> build.cmd -release -- /p:CoreCLROverridePath=C:\git\coreclr\bin\Product\Windows_NT.x64.Release\
C:\git\corefx> msbuild src\System.Collections\tests\ /t:rebuildandtest /p:ConfigurationGroup:release

@MarcoRossignoli
Copy link
Copy Markdown
Member Author

@dotnet-bot test Tizen armel Cross Checked Innerloop Build and Test please

get
{
return _comparer;
return (_comparer is NonRandomizedStringEqualityComparer) ? (IEqualityComparer<TKey>)EqualityComparer<string>.Default : _comparer;
Copy link
Copy Markdown
Member

@stephentoub stephentoub Feb 2, 2018

Choose a reason for hiding this comment

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

Shouldn't this be EqualityComparer<TKey> rather than EqualityComparer<string>? Maybe we're just not testing this code path?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, I see why it works ok... TKey must be a string for _comparer to get set to NonRandomizedStringEqualityComparer.

Copy link
Copy Markdown
Member Author

@MarcoRossignoli MarcoRossignoli Feb 2, 2018

Choose a reason for hiding this comment

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

actually compile also with TKey because the constraints are respected and for now we've only optimization with string TKey, i think with EqualityComparer of string the intentions are more clear. Do you agree?

Copy link
Copy Markdown
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks!

@jkotas jkotas merged commit 8aa7f12 into dotnet:master Feb 3, 2018
dotnet-bot pushed a commit to dotnet/corert that referenced this pull request Feb 3, 2018
Fixes dotnet/corefx#26033

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
dotnet-bot pushed a commit to dotnet/corefx that referenced this pull request Feb 3, 2018
Fixes dotnet/corefx#26033

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
jkotas pushed a commit to dotnet/corefx that referenced this pull request Feb 3, 2018
Fixes dotnet/corefx#26033

Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
jkotas pushed a commit to dotnet/corert that referenced this pull request Feb 3, 2018
Fixes dotnet/corefx#26033

Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
@MarcoRossignoli MarcoRossignoli deleted the corefx_26033 branch February 3, 2018 07:43
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Fixes dotnet/corefxdotnet/coreclr#26033

Commit migrated from dotnet/coreclr@8aa7f12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants