Partial clean up of V3Order. No functional change intended.

This is a partial cleanup of V3Order with the aim of increasing clarity:
- Split the initial OrderGraph building and the actual ordering process
into separate classes (OrderVisitor -> OrderBuildVisitor + OrderProcess)
- Remove all the historical cruft from the graph building phase (now in
OrderBuildVisitor), and add more assertions for assumptions.
- Change the dot styling of OrderGraph to use shapes and more easily
distinguishable colors.
- Expand vague comments, remove incorrect comments, and add more.
- Replace some old code with cleaner C++11 constructs.
- Move code about a bit so logically connected sections are closer to
each other, scope some definitions where they are used rather than file
scope.
- The actual ordering process (now in OrderProcess) is still largely
unchanged.

The generated code is identical to before (within the limits of the
exiting non-determinism).

src/V3Ast.cpp

@@ -1276,9 +1276,9 @@ AstNodeDType* AstNode::findVoidDType() const {
 }
 
 //######################################################################
-// AstNVisitor
+// AstNDeleter
 
-void AstNVisitor::doDeletes() {
+void AstNDeleter::doDeletes() {
-    for (AstNode* nodep : m_deleteps) nodep->deleteTree();
+    for (AstNode* const nodep : m_deleteps) nodep->deleteTree();
     m_deleteps.clear();
 }

src/V3Ast.h

@@ -1251,31 +1251,39 @@ public:
 // clang-format on
 
 //######################################################################
-// AstNVisitor -- Allows new functions to be called on each node
+// Node deleter, deletes all enqueued AstNode* on destruction, or when
-// type without changing the base classes.  See "Modern C++ Design".
+// explicitly told to do so. This is useful when the deletion of removed
+// nodes needs to be deferred to a later time, because pointers to the
+// removed nodes might still exist.
 
-class AstNVisitor VL_NOT_FINAL {
+class AstNDeleter VL_NOT_FINAL {
-private:
     // MEMBERS
-    std::vector<AstNode*> m_deleteps;  // Nodes to delete when doDeletes() called
+    std::vector<AstNode*> m_deleteps;  // Nodes to delete
-protected:
-    friend class AstNode;
 
 public:
     // METHODS
-    /// At the end of the visitor (or doDeletes()), delete this pushed node
+
-    /// along with all children and next(s). This is often better to use
+    // Enqueue node for deletion on next 'doDelete' (or destruction)
-    /// than an immediate deleteTree, as any pointers into this node will
-    /// persist for the lifetime of the visitor
     void pushDeletep(AstNode* nodep) {
         UASSERT_STATIC(nodep, "Cannot delete nullptr node");
         m_deleteps.push_back(nodep);
     }
-    /// Call deleteTree on all previously pushDeletep()'ed nodes
+
+    // Delete all previously pushed nodes (by callint deleteTree)
     void doDeletes();
 
+    // Do the deletions on destruction
+    virtual ~AstNDeleter() { doDeletes(); }
+};
+
+//######################################################################
+// AstNVisitor -- Allows new functions to be called on each node
+// type without changing the base classes.  See "Modern C++ Design".
+
+class AstNVisitor VL_NOT_FINAL : public AstNDeleter {
+    friend class AstNode;
+
 public:
-    virtual ~AstNVisitor() { doDeletes(); }
     /// Call visit()s on nodep
     void iterate(AstNode* nodep);
     /// Call visit()s on nodep

src/V3AstNodes.h

@@ -2300,9 +2300,9 @@ class AstScope final : public AstNode {
 private:
     // An AstScope->name() is special: . indicates an uninlined scope, __DOT__ an inlined scope
     string m_name;  // Name
-    AstScope* m_aboveScopep;  // Scope above this one in the hierarchy (nullptr if top)
+    AstScope* const m_aboveScopep;  // Scope above this one in the hierarchy (nullptr if top)
-    AstCell* m_aboveCellp;  // Cell above this in the hierarchy (nullptr if top)
+    AstCell* const m_aboveCellp;  // Cell above this in the hierarchy (nullptr if top)
-    AstNodeModule* m_modp;  // Module scope corresponds to
+    AstNodeModule* const m_modp;  // Module scope corresponds to
 public:
     AstScope(FileLine* fl, AstNodeModule* modp, const string& name, AstScope* aboveScopep,
              AstCell* aboveCellp)
@@ -2321,8 +2321,8 @@ public:
     string nameDotless() const;
     string nameVlSym() const { return ((string("vlSymsp->")) + nameDotless()); }
     AstNodeModule* modp() const { return m_modp; }
-    void addVarp(AstNode* nodep) { addOp1p(nodep); }
+    void addVarp(AstVarScope* nodep) { addOp1p((AstNode*)nodep); }
-    AstNode* varsp() const { return op1p(); }  // op1 = AstVarScope's
+    AstVarScope* varsp() const { return VN_AS(op1p(), VarScope); }  // op1 = AstVarScope's
     void addActivep(AstNode* nodep) { addOp2p(nodep); }
     AstNode* blocksp() const { return op2p(); }  // op2 = Block names
     void addFinalClkp(AstNode* nodep) { addOp3p(nodep); }
@@ -2367,6 +2367,8 @@ public:
         : ASTGEN_SUPER_VarScope(fl)
         , m_scopep{scopep}
         , m_varp{varp} {
+        UASSERT_OBJ(scopep, fl, "Scope must be non-null");
+        UASSERT_OBJ(varp, fl, "Var must be non-null");
         m_circular = false;
         m_trace = true;
         dtypeFrom(varp);

src/V3Class.cpp

@@ -141,8 +141,9 @@ public:
             if (VN_IS(moved.first, NodeFTask)) {
                 moved.second->addActivep(moved.first->unlinkFrBack());
             } else if (VN_IS(moved.first, Var)) {
-                AstVarScope* scopep = VN_AS(moved.first->user1p(), VarScope);
+                AstVarScope* const scopep = VN_AS(moved.first->user1p(), VarScope);
-                moved.second->addVarp(scopep->unlinkFrBack());
+                scopep->unlinkFrBack();
+                moved.second->addVarp(scopep);
             }
         }
     }

src/V3OrderGraph.h

@@ -167,6 +167,7 @@ public:
     virtual string name() const override { return "*INPUTS*"; }
     virtual string dotColor() const override { return "green"; }
     virtual string dotName() const override { return ""; }
+    virtual string dotShape() const override { return "invhouse"; }
     virtual bool domainMatters() override { return false; }
 };
 
@@ -193,7 +194,9 @@ public:
         return (cvtToHex(m_nodep) + "\\n " + cvtToStr(nodep()->typeName()));
     }
     AstNode* nodep() const { return m_nodep; }
-    virtual string dotColor() const override { return "yellow"; }
+    virtual string dotShape() const override {
+        return VN_IS(m_nodep, Active) ? "doubleoctagon" : "rect";
+    }
 };
 
 class OrderVarVertex VL_NOT_FINAL : public OrderEitherVertex {
@@ -221,6 +224,7 @@ public:
     bool isClock() const { return m_isClock; }
     void isDelayed(bool flag) { m_isDelayed = flag; }
     bool isDelayed() const { return m_isDelayed; }
+    virtual string dotShape() const override { return "ellipse"; }
 };
 
 class OrderVarStdVertex final : public OrderVarVertex {
@@ -238,7 +242,7 @@ public:
     virtual string name() const override {
         return (cvtToHex(varScp()) + "\\n " + varScp()->name());
     }
-    virtual string dotColor() const override { return "skyblue"; }
+    virtual string dotColor() const override { return "grey"; }
     virtual bool domainMatters() override { return true; }
 };
 class OrderVarPreVertex final : public OrderVarVertex {
@@ -256,7 +260,7 @@ public:
     virtual string name() const override {
         return (cvtToHex(varScp()) + " PRE\\n " + varScp()->name());
     }
-    virtual string dotColor() const override { return "lightblue"; }
+    virtual string dotColor() const override { return "green"; }
     virtual bool domainMatters() override { return false; }
 };
 class OrderVarPostVertex final : public OrderVarVertex {
@@ -274,7 +278,7 @@ public:
     virtual string name() const override {
         return (cvtToHex(varScp()) + " POST\\n " + varScp()->name());
     }
-    virtual string dotColor() const override { return "CadetBlue"; }
+    virtual string dotColor() const override { return "red"; }
     virtual bool domainMatters() override { return false; }
 };
 class OrderVarPordVertex final : public OrderVarVertex {
@@ -292,7 +296,7 @@ public:
     virtual string name() const override {
         return (cvtToHex(varScp()) + " PORD\\n " + varScp()->name());
     }
-    virtual string dotColor() const override { return "NavyBlue"; }
+    virtual string dotColor() const override { return "blue"; }
     virtual bool domainMatters() override { return false; }
 };
 
@@ -307,7 +311,7 @@ class OrderMoveVertex final : public V3GraphVertex {
     OrderMoveDomScope* m_domScopep;  // Domain/scope list information
 
 protected:
-    friend class OrderVisitor;
+    friend class OrderProcess;
     friend class OrderMoveVertexMaker;
     // These only contain the "next" item,
     // for the head of the list, see the same var name under OrderVisitor

src/V3SenTree.h

@@ -82,21 +82,24 @@ private:
 public:
     // CONSTRUCTORS
     SenTreeFinder()
-        : m_topScopep{v3Global.rootp()->topScopep()} {
+        : SenTreeFinder(v3Global.rootp()) {}
+
+    explicit SenTreeFinder(AstNetlist* netlistp)
+        : m_topScopep{netlistp->topScopep()} {
         // Gather existing global SenTrees
-        for (AstSenTree* nodep = m_topScopep->senTreesp(); nodep;
+        for (AstNode* nodep = m_topScopep->senTreesp(); nodep; nodep = nodep->nextp()) {
-             nodep = VN_AS(nodep->nextp(), SenTree)) {
+            m_trees.add(VN_AS(nodep, SenTree));
-            m_trees.add(nodep);
         }
     }
 
     // METHODS
+
+    // Return a global AstSenTree that matches given SenTree.
+    // If no such global AstSenTree exists create one and add it to the stored AstTopScope.
     AstSenTree* getSenTree(AstSenTree* senTreep) {
-        // Return a global SenTree that matches given SenTree. If no such global
-        // SenTree exists, create one and add it to the stored TopScope.
         AstSenTree* treep = m_trees.find(senTreep);
-        // Not found, form a new one
         if (!treep) {
+            // Not found, form a new one
             treep = senTreep->cloneTree(false);
             m_topScopep->addSenTreep(treep);
             UINFO(8, "    New SENTREE " << treep << endl);
@@ -104,6 +107,16 @@ public:
         }
         return treep;
     }
+
+    // Return the global combinational AstSenTree.
+    // If no such global SenTree exists create one and add it to the stored AstTopScope.
+    AstSenTree* getComb() {
+        FileLine* const fl = m_topScopep->fileline();
+        AstSenTree* const combp = new AstSenTree{fl, new AstSenItem{fl, AstSenItem::Combo()}};
+        AstSenTree* const resultp = getSenTree(combp);
+        VL_DO_DANGLING(combp->deleteTree(), combp);  // getSenTree clones, so can delete
+        return resultp;
+    }
 };
 
 #endif  // Guard

src/astgen

@@ -362,21 +362,19 @@ def read_types(filename):
             match = re.search(r'^\s*(class|struct)\s*(\S+)', line)
             if match:
                 classn = match.group(2)
-                inh = ""
                 match = re.search(r':\s*public\s+(\S+)', line)
-                if match:
+                supern = match.group(1) if match else ""
-                    inh = match.group(1)
+                assert classn != "AstNode" or supern == "", "AstNode can't have a superclass"
-                # print("class "+classn+" : "+inh)
+                if re.search(r'Ast', supern) or classn == "AstNode":
-                if classn == "AstNode":
+                    if supern == "AstNDeleter":
-                    inh = ""
+                        continue
-                if re.search(r'Ast', inh) or classn == "AstNode":
                     classn = re.sub(r'^Ast', '', classn)
-                    inh = re.sub(r'^Ast', '', inh)
+                    supern = re.sub(r'^Ast', '', supern)
-                    Classes[classn] = inh
+                    Classes[classn] = supern
-                    if inh != '':
+                    if supern != '':
-                        if inh not in Children:
+                        if supern not in Children:
-                            Children[inh] = {}
+                            Children[supern] = {}
-                        Children[inh][classn] = 1
+                        Children[supern][classn] = 1
 
 
 def read_stages(filename):