Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor option evaluation logic #32

Merged
merged 1 commit into from
Aug 3, 2017
Merged

Refactor option evaluation logic #32

merged 1 commit into from
Aug 3, 2017

Conversation

dsnet
Copy link
Collaborator

@dsnet dsnet commented Aug 1, 2017

The previous implementation of options had a single type "option"
that was used to represent either an Ignore, Comparer, or Transformer
and all of the filters relevant to each of them.

We refactor this logic by creating a new type to represent each
fundamental option and each filtering option. Construction of
filtered options is now as simple as wrapping the input option with
the appropriate filter type.

Evaluation of filters now takes a top-down approach where each of
the options now have a filter method and know how to filter themselves
and recursively call filter on the any sub-options.

This approach has the following advantages:

  • It is faster because the same filter that was applied to multiple
    options now only needs to execute once.
  • Even though this is semantically identical, the implementation more
    closely matches the documented algorithm in cmp.Equal.
  • It allows for easier extension of the API to add new fundamental
    and filtering option types.

We also add a String method to each of the fundamental and filter options.
This adds to the complexity of this commit, but helps users with debugging
since printing an option shows the full tree of everything applied.

@dsnet dsnet requested a review from neild August 1, 2017 17:24
@dsnet dsnet force-pushed the refactor-options branch 2 times, most recently from 5ed4337 to 7c43715 Compare August 1, 2017 19:47
cmp/compare.go Outdated

func (s *state) applyOption(vx, vy reflect.Value, t reflect.Type, opt option) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can/should this function be replaced with a method on the Option interface?

type Option interface {
  apply(vx, vy reflect.Value, t reflect.Type)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would need to be:

apply(p Path, vx, vy reflect.Value, t reflect.Type) []Option

I don't mind plumbing Path all the time, but requiring every possible applicable option to allocate a slice seems very non-performant. For that reason, evaluate is method on optionEvaluator which only has a single slice to hold all that information.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We might be able to avoid the performance hit by returning a single Option and relying on Options for the rare case where multiple are applicable. Let me look into this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could plumb the *state instead of the Path.

This is all internal, so it doesn't matter that much, but it's weird to have an interface implemented by N types and then a big type switch on those types; isn't that precisely what interface methods are for?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

cmp/options.go Outdated
out = opt
case invalid:
// Keep invalid
case *comparer, *transformer:
Copy link
Collaborator Author

@dsnet dsnet Aug 1, 2017

Choose a reason for hiding this comment

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

This is incorrect. I have to batch all *comparers and *transformers since it is still possible that a later Ignore will take precedence over all these conflicts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed. I made it such that filter can return a Options only when there is a conflict.

@dsnet dsnet force-pushed the refactor-options branch 12 times, most recently from 5e3fb58 to 138211a Compare August 3, 2017 00:29
@dsnet
Copy link
Collaborator Author

dsnet commented Aug 3, 2017

I did your suggestion of removing the type switch adding each case as a method.

The reason I was confused was because I thought you were talking about adding that functionality to the filter method, which doesn't work.

I do like the extra bit of type safety this brings, where the return type of filter is applicableOption.

The previous implementation of options had a single type "option"
that was used to represent either an Ignore, Comparer, or Transformer
and all of the filters relevant to each of them.

We refactor this logic by creating a new type to represent each of
the fundamental options and filtering options. Construction of
filtered options is now as simple as wrapping the input option with
the appropriate filter type.

Evaluation of filters now takes a top-down approach where we start
with the set of all options, and recursively apply the filters
to create the "applicable" set S.

This approach has the following advantages:
* It is faster because the same filter that was applied to multiple
options now only needs to execute once.
* It more closely matches the documented algorithm in cmp.Equal.
* It allows for easier extension of the API to add new fundamental
option types.
@dsnet dsnet merged commit 8099a97 into master Aug 3, 2017
@dsnet dsnet deleted the refactor-options branch August 3, 2017 17:35
Sign up for free to 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