Move Node-specific behavior into Node subclasses #1039
Conversation
…f the Parent is null
…t be canonically equivalent to `e + 0`
|
|
||
| // Cleanup the memory consumed by this node. | ||
| // @param[in] this is the current node of the AST. | ||
| virtual void Cleanup() { |
There was a problem hiding this comment.
The Node class must have a virtual destructor otherwise when you invoke delete on the derived class object the base class destructor won't be called. I also see a compile-time warning for this:
PreorderAST.cpp:410:14: warning: deleting object of polymorphic class type ‘clang::Node’ which has non-virtual destructor might cause undefined behavior [-Wdelete-non-virtual-dtor] 410 | delete(*I);
| // @param[in] Changed indicates whether a node was coalesced. We need this | ||
| // to control when to stop recursive coalescing. | ||
| // @param[in] Error indicates whether an error occurred during coalescing. | ||
| virtual void Coalesce(bool &Changed, bool &Error) { } |
There was a problem hiding this comment.
Ideally we should make any function which does not have an implementation in the base class a pure virtual function meaning that its implementation is present only in the derived class. For example:
virtual void Coalesce(bool &Changed, bool &Error) = 0;
But that also means we would not be able to instantiate the Node class which is not something we want.
…ex-based for loop This fixes a crash that would happen on Linux for certain binary operators.
| // Since Children is modified within the loop, we need to evaluate | ||
| // the loop end on each iteration. | ||
| for (size_t I = 0; I != Children.size(); ++I) { | ||
| auto *Child = Children[I]; |
There was a problem hiding this comment.
There is still an incorrectness here - Consider this scenario:
In the third iteration of the loop, I = 2. Child will be Children[2]. Let the coalesce function called on Child (which is now the same as Children[2]) erase Child. Because of this erasure, Children[2] will now contain what was previously Children[3]. Then the control comes out of the Coalesce function and I gets incremented to 3. In the next iteration of the loop, Children[3] will be processed while Children[2] never got processed.
There was a problem hiding this comment.
Discussed offline. Summary:
Not every child of a BinaryOperatorNode B will be processed in a call to B->Coalesce(). Any children of B that were not processed during a call to B->Coalesce() will be processed during future calls to B->Coalesce() in the PreorderAST::Normalize method, which calls Root->Coalesce(), Root->Sort(), and Root->ConstantFold() in a while loop while a Changed flag is true.
In the future, we may want to rewrite the for (size_t I = 0; I != Children.size(); ++I) loop on line 259 so that all children of B are processed in a single call to B->Coalesce(). This would be done in separate PR(s).
clang/lib/AST/PreorderAST.cpp
Outdated
| // Remove the current node from the list of children of its parent. | ||
| // Since BParent is modified within the loop, we need to evaluate | ||
| // the loop end on each iteration. | ||
| for (auto I = BParent->Children.begin(); I != BParent->Children.end(); ++I) { |
There was a problem hiding this comment.
Actually, as soon as the loop is modified we break out of the loop. So we don't really need to evaluate BParent->Children.end() on each iteration. This can be changed to:
for (auto I = BParent->Children.begin(), E != BParent->Children.end(); I != E; ++I)
…pure virtual Node functions
Fixes #1032
This PR refactors PreorderAST to move Coalesce, Sort, ConstantFold, Compare, Cleanup, and PrettyPrint behavior into Node subclass methods.