TopoSorter: Fix sorting of container template parameters

For std::vector and std::list, template parameters are not required to
be defined before they can be used. Delay sorting them until the end.

Also fix a CodeGen bug where we were defining typedefs in the middle of
the forward declarations. They only need to be defined when other types
are defined.

Author: ajor

oi/CodeGen.cpp

@@ -150,18 +150,12 @@ void genDeclsEnum(const Enum& e, std::string& code) {
   code += ";\n";
 }
 
-void genDeclsTypedef(const Typedef& td, std::string& code) {
-  code += "using " + td.name() + " = " + td.underlyingType()->name() + ";\n";
-}
-
 void genDecls(const TypeGraph& typeGraph, std::string& code) {
   for (const Type& t : typeGraph.finalTypes) {
     if (const auto* c = dynamic_cast<const Class*>(&t)) {
       genDeclsClass(*c, code);
     } else if (const auto* e = dynamic_cast<const Enum*>(&t)) {
       genDeclsEnum(*e, code);
-    } else if (const auto* td = dynamic_cast<const Typedef*>(&t)) {
-      genDeclsTypedef(*td, code);
     }
   }
 }

oi/type_graph/TopoSorter.cpp

@@ -55,29 +55,57 @@ void TopoSorter::visit(Type& type) {
 }
 
 void TopoSorter::visit(Class& c) {
-  for (const auto& param : c.templateParams) {
-    visit(param.type);
-  }
   for (const auto& parent : c.parents) {
     visit(*parent.type);
   }
   for (const auto& mem : c.members) {
     visit(*mem.type);
   }
+  for (const auto& param : c.templateParams) {
+    visit(param.type);
+  }
   sortedTypes_.push_back(c);
 
-  // Same as pointers, child do not create a dependency so are delayed until the
-  // end
+  // Same as pointers, children do not create a dependency so are delayed until
+  // the end.
   for (const auto& child : c.children) {
-    typesToSort_.push(child);
+    visitAfter(child);
+  }
+}
+
+namespace {
+/*
+ * C++17 allows the std::vector, std::list and std::forward_list containers to
+ * be instantiated with incomplete types.
+ *
+ * Other containers are not required to do this, but might still have this
+ * behaviour.
+ */
+bool containerAllowsIncompleteParams(const Container& c) {
+  switch (c.containerInfo_.ctype) {
+    case SEQ_TYPE:
+    case LIST_TYPE:
+      // Also std::forward_list, if we ever support that
+      // Would be good to have this as an option in the TOML files
+      return true;
+    default:
+      return false;
   }
 }
+}  // namespace
 
 void TopoSorter::visit(Container& c) {
-  for (const auto& param : c.templateParams) {
-    visit(param.type);
+  if (!containerAllowsIncompleteParams(c)) {
+    for (const auto& param : c.templateParams) {
+      visit(param.type);
+    }
   }
   sortedTypes_.push_back(c);
+  if (containerAllowsIncompleteParams(c)) {
+    for (const auto& param : c.templateParams) {
+      visitAfter(param.type);
+    }
+  }
 }
 
 void TopoSorter::visit(Enum& e) {
@@ -92,7 +120,25 @@ void TopoSorter::visit(Typedef& td) {
 void TopoSorter::visit(Pointer& p) {
   // Pointers do not create a dependency, but we do still care about the types
   // they point to, so delay them until the end.
-  typesToSort_.push(*p.pointeeType());
+  visitAfter(*p.pointeeType());
+}
+
+/*
+ * A type graph may contain cycles, so we need to slightly tweak the standard
+ * topological sorting algorithm. Cycles can only be introduced by certain
+ * edges, e.g. a pointer's underlying type. However, these edges do not
+ * introduce dependencies as these types can be forward declared in a C++
+ * program. This means we can delay processing them until after all of the true
+ * dependencies have been sorted.
+ */
+void TopoSorter::visitAfter(Type& type) {
+  typesToSort_.push(type);
+}
+
+void TopoSorter::visitAfter(Type* type) {
+  if (type) {
+    visitAfter(*type);
+  }
 }
 
 }  // namespace type_graph

oi/type_graph/TopoSorter.h

@@ -51,6 +51,9 @@ class TopoSorter : public RecursiveVisitor {
   std::unordered_set<Type*> visited_;
   std::vector<std::reference_wrapper<Type>> sortedTypes_;
   std::queue<std::reference_wrapper<Type>> typesToSort_;
+
+  void visitAfter(Type& type);
+  void visitAfter(Type* type);
 };
 
 }  // namespace type_graph

test/test_topo_sorter.cpp

@@ -153,11 +153,41 @@ MyChild
 }
 
 TEST(TopoSorterTest, Containers) {
+  auto myparam1 = Class{Class::Kind::Struct, "MyParam1", 13};
+  auto myparam2 = Class{Class::Kind::Struct, "MyParam2", 13};
+  auto mycontainer = getMap();
+  mycontainer.templateParams.push_back((&myparam1));
+  mycontainer.templateParams.push_back((&myparam2));
+
+  test({mycontainer}, R"(
+MyParam1
+MyParam2
+std::map
+)");
+}
+
+TEST(TopoSorterTest, ContainersVector) {
+  // std::vector allows forward declared template parameters
   auto myparam = Class{Class::Kind::Struct, "MyParam", 13};
   auto mycontainer = getVector();
   mycontainer.templateParams.push_back((&myparam));
 
-  test({mycontainer}, "MyParam\nstd::vector");
+  test({mycontainer}, R"(
+std::vector
+MyParam
+)");
+}
+
+TEST(TopoSorterTest, ContainersList) {
+  // std::list allows forward declared template parameters
+  auto myparam = Class{Class::Kind::Struct, "MyParam", 13};
+  auto mycontainer = getList();
+  mycontainer.templateParams.push_back((&myparam));
+
+  test({mycontainer}, R"(
+std::list
+MyParam
+)");
 }
 
 TEST(TopoSorterTest, Arrays) {

test/type_graph_utils.cpp

@@ -59,3 +59,15 @@ Container getVector() {
   info.stubTemplateParams = {1};
   return Container{info, 24};
 }
+
+Container getMap() {
+  static ContainerInfo info{"std::map", STD_MAP_TYPE, "map"};
+  info.stubTemplateParams = {2, 3};
+  return Container{info, 48};
+}
+
+Container getList() {
+  static ContainerInfo info{"std::list", LIST_TYPE, "list"};
+  info.stubTemplateParams = {1};
+  return Container{info, 24};
+}

test/type_graph_utils.h

@@ -24,3 +24,5 @@ void test(type_graph::Pass pass,
           std::string_view expectedAfter);
 
 type_graph::Container getVector();
+type_graph::Container getMap();
+type_graph::Container getList();