This repository has been archived by the owner on Apr 28, 2023. It is now read-only.
ScheduleTree: replace comparison operators with named functions #583
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The mapping between identifiers and affine functions was introduced to the ScheduleTreeMapping in a630d04 (ScheduleTreeElemMappingFilter: store mapping between identifiers and functions, Wed May 16 17:44:08 2018 +0200), however the comparison operator was only comparing the union_set filter_. While the two representations are generally expected to be equivalent, it is possible to modify them individually. So we need to also compare the mappings in ScheduleTreeMapping::operator==.
This method is abstract in the base class and is defined in derived classes to perform the type comparison and, if types match, call the class-specific comparison operator (or return true immediately for tree nodes that are always considered equal to each other, e.g., sequences). Its name indicates that that it applies to the current node only, ignoring the subtree that may be rooted at this node. It will be used in the upcoming commits to use virtual methods instead of macro-based dispatch on schedule node types.
The elemEquals function, as its name suggests, was introduced when ScheduleTreeElement was a separate concept. It was kept mostly intact to minimize the changes when individual tree node classes were made inheriting ScheduleTree instead. Replace elemEquals with a call to the virtual function nodeEquals that leverages polymorphism to perform dynmaic dispatch instead of manual macro-based one.
In tree-like structures, the behavior of the default comparison operators (operator== and operator!=) are not intuitive. They may compare individual tree nodes or the subtrees rooted at those nodes. The current implementation made this confusion even worse where ScheduleTree::operator== compared subtrees but ScheduleTree*::operator== did not. Replace comparison operators by type-safe non-virtual overloads of nodeEquals in specific tree node classes. Subtree comparison can be performed by a dedicated method that will be introduced next.
In tree-like structures, the behavior of the default comparison operators (operator== and operator!=) is not intuitive. They may compare individual tree nodes or the subtrees rooted at those nodes. Replace the equality comparison operator on ScheduleTree with the treeEquals method, which makes it clear that subtrees are compared (as opposed to nodeEquals introduced previously). Removing the overloaded comparison operators may make it harder to use standard containers and algorithms on ScheduleTrees. However, the caller is never supposed to operate on ScheduleTrees by-value, and pointers are trivially comparable. Internal functions may define and use a comparator class with clear intended behavior when necessary. For external uses, explicitly-named functions offer a better alternative.
ftynse
changed the title
nicolasvasilache
approved these changes
Dec 11, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proceeding with ScheduleTree evolution plan (#553)
This PR replaces overloaded comparison operators with two functions,
nodeEqualsandtreeEquals. The main motivation for this change: after grafting ScheduleTreeElem* onto ScheduleTree in the inheritance structure we ended up withoperator==comparing subtrees if called on ScheduleTree and nodes if called on instances of other classes. I argue that the behavior of default comparison operator is unintutitive for tree-like structure. Therefore, introducing explicitly-named comparison functions that make the intention clear is an improvement.As an additional benefit, regular member functions can be declared virtual, unlike operators. With this approach, each subclass needs to override one abstract function that does node-to-node comparison while subtree comparison is handed automatically. Previously, it would have been necessary to also modify the clunky macro-based logic in one of the comparison operators.