Skip to content

Conversation

@zii-dmg
Copy link
Contributor

Make comparisons by reference in TokenPartitioner. Fixes#398.

Make comparisons by reference in TokenPartitioner.
.Where(t =>!sequences.Contains(t,ReferenceEqualityComparer.Default)).Memorize();
varvalues=nonOptions.Where(v =>v.IsValue()).Memorize();
varerrors=nonOptions.Except(values).Memorize();
varerrors=nonOptions.Except(values,ReferenceEqualityComparer.Default).Cast<Token>().Memorize();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you casting back to Token because the comparer casts everything into Object? What if you made the comparer inherit IEqualityComparer<Token> and then called [Object.ReferenceEquals](https://msdn.microsoft.com/en-us/library/system.object.referenceequals(v=vs.110\).aspx) in the implementation rather than ==? That's a bit more explicit too, and wouldn't need a comment explaining that we're aiming for reference equality on line 16.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I realized that I can make other cast - (IEqualityComparer<Token>)ReferenceEqualityComparer.Default so Cast<Token> will not be needed. Or can make modify comparer to classic generic version - ReferenceEqualityComparer<T>. Don't want to inherit IEqualityComparer<Token>, it's too concrete for this task.

Will replace == with ReferenceEquals.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Generic sounds good to me. Any time we can make the type system work for us is a win in my book 😃

Copy link
ContributorAuthor

@zii-dmgzii-dmgJan 16, 2017

Choose a reason for hiding this comment

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

But reference comparer doesn't have to be generic because it compares references anyway. I used this implementation from stackoverflow. As author says it's using contravariance feature so we can cast comparer to any type of IEqualityComparer<T> and we have one instance of comparer for any object type.

@zii-dmg
Copy link
ContributorAuthor

If you really want ReferenceEqualityComparer<T> I will make it, just say. Waiting for answer.

@nemecnemec merged commit 27243c4 into gsscoder:masterJan 18, 2017
@nemec
Copy link
Collaborator

Looks good

@zii-dmg
Copy link
ContributorAuthor

I accidently removed FactAttribute from last test with merge. Can you restore it?

@nemec
Copy link
Collaborator

Fixed

@zii-dmgzii-dmg deleted the same-value-option-arg branch January 20, 2017 15:44
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@zii-dmg@nemec