ScheduleTree API evolution

[ftynse]

#543 removed the notion of schedule tree element: now specific, node types inherit directly from ScheduleTree, which is simpler and offers more type safety. This refactoring uncovered several technical and conceptual issues that may be worth discussing.

• Using macros for dynamic dispatch.
  I don't think using macros for type-based dispatch is justified in C++. Virtual functions are a native mechanism to achieve exactly that behavior. Worth, some functions already rely on dynamic polymorphism through virtual functions while others do macro-based dynamic dispatch. We can also consider true CRTP for static dispatch, but it looks like premature optimization: virtual function calls are cheap.
• The type_ field.
  If we get rid of macro-based dispatch, the type_ field is not strictly necessary. One can use dynamic_cast to check if a schedule node has in fact a specific node type. Alternatively, the field may be replaced with a virtual function overridden by derived classes to return a constant (note that ScheduleTree subclasses #543 killed such a function, but it was duplicate data and used in much less places than the type_ field). With CRTP, the subclass type is readily available in the parent class.
• Separate header containing all specific node types.
  Not sure if it justified to keep classes specific node types in a separate file (schedule_tree_elem.{h,cc}). Most tree utils and transformations require specific node classes anyway, for example to extract the domain space from the domain node. The only functions that may not require the definitions of specific node types are generic traversals, which are static members of ScheduleTree anyway.
• Construction management
  Specific node classes have public constructors, but the memory management scheme for ScheduleTree says that one should manipulate ScheduleTrees through (unique) pointers only. ScheduleTree has a set of static member functions make* to construct specific nodes wrapped in a unique pointer. Given the new inheritance scheme, one can move these static constructor functions to the respective classes, removing the need for public constructors. Alternatively, derived classes may declare ScheduleTree as friend, but the constructors should be hidden anyway.

tag @nicolasvasilache @skimo-openhub for discussion

[ftynse]

My preferences for these are:

• use virtual methods, kill macro-dispatch with fire
• use a virtual type() method
• merge schedule_tree.h and schedule_tree_elem.h
• move construction to specific classes, add a virtual clone() method to properly dispatch copy constuction

[nicolasvasilache]

Macros are just a way to avoid copy-pasting code. The real question is whether we should try to kill dynamic dispatch everywhere (i.e. no switch or if-else based on type). In general yes if we can (i.e. use virtual functions, as discussed offline, where it makes sense; e.g. pprint)

virtual enum type(); vs enum type_; is an easy one: favor killing state so virtual function all the way.

dynamic_cast requires RTTI with which we had some issues in the past (can't remember the details offhand). I don't think it buys us much over using as so long as we don't have a class hierarchy. I'd prefer to avoid dynamic_cast and typeid unless it's the best solution.

Construction management is a natural extension of what you did in #543 so go for it.

Merging files, I'd also vote for now that we have derived types.

So overall, agreed, but I have the impression we will have a few places with if-else or switch and code that is very similar (but I may be wrong). In that case macros will still beat copy-pasta.

We also discussed the visitor pattern like in Halide but I think this is premature here: we don't have enough types and as @ftynse was mentioning we may just want to kill (almost) all types at some point.

[ftynse]

So overall, agreed, but I have the impression we will have a few places with if-else or switch and code that is very similar (but I may be wrong). In that case macros will still beat copy-pasta.

Let's see case-by-case. If there is common behavior, it is often possible to abstract it away differently than by copy-pasting (be it macros or not). The only thing that I now for sure must be made with macro is generating function/class names like I did here https://github.com/PollyLabs/islutils/blob/01c947458af451034e83df28f230730347863aec/islutils/matchers-inl.h#L53