Responded to some comments by @ZedThree

The comments were in PR #445, but some of them actually relate to work
done in #428. Only the latter are dealt with in this PR.

Author: cmacmackin

include/component.hxx

@@ -1,17 +1,7 @@
 #pragma once
-
 #ifndef HERMES_COMPONENT_H
 #define HERMES_COMPONENT_H
 
-#include <bout/assert.hxx>
-#include <bout/bout_types.hxx>
-#include <bout/boutexception.hxx>
-#include <bout/field2d.hxx>
-#include <bout/field3d.hxx>
-#include <bout/generic_factory.hxx>
-#include <bout/options.hxx>
-#include <bout/unused.hxx>
-
 #include <cmath>
 #include <initializer_list>
 #include <map>
@@ -22,6 +12,15 @@
 #include <utility>
 #include <vector>
 
+#include <bout/assert.hxx>
+#include <bout/bout_types.hxx>
+#include <bout/boutexception.hxx>
+#include <bout/field2d.hxx>
+#include <bout/field3d.hxx>
+#include <bout/generic_factory.hxx>
+#include <bout/options.hxx>
+#include <bout/unused.hxx>
+
 #include "guarded_options.hxx"
 #include "permissions.hxx"
 #include "hermes_utils.hxx"
@@ -34,8 +33,10 @@ struct SpeciesInformation {
   SpeciesInformation(const std::vector<std::string>& electrons,
                      const std::vector<std::string>& neutrals,
                      const std::vector<std::string>& positive_ions,
-                     const std::vector<std::string> & negative_ions)
-    : electrons(electrons), neutrals(neutrals), positive_ions(positive_ions), negative_ions(negative_ions), ions(positive_ions) {
+                     const std::vector<std::string>& negative_ions)
+      : electrons(std::move(electrons)), neutrals(std::move(neutrals)),
+        positive_ions(std::move(positive_ions)), negative_ions(std::move(negative_ions)),
+        ions(std::move(positive_ions)) {
     finish_construction();
   }
 
@@ -112,10 +113,9 @@ struct Component {
   /// @param name     The species/name for this instance.
   /// @param options  Component settings: options[name] are specific to this component
   /// @param solver   Time-integration solver
-  static std::unique_ptr<Component> create(const std::string &type, // The type to create
-                                           const std::string &name, // The species/name for this instance
-                                           Options &options,  // Component settings: options[name] are specific to this component
-                                           Solver *solver); // Time integration solver
+  static std::unique_ptr<Component> create(const std::string& type,
+                                           const std::string& name, Options& options,
+                                           Solver* solver);
 
   /// Tell the component the name of all species in the simulation, by type. It
   /// will use this information to substitute the following placeholders in
@@ -451,14 +451,15 @@ template<typename T>
 Options& add(Options& option, T value) {
   if (!option.isSet()) {
     return set(option, value);
-  } else {
-    try {
-      return set(option, value + bout::utils::variantStaticCastOrThrow<Options::ValueType, T>(option.value));
-    } catch (const std::bad_cast &e) {
-      // Convert to a more useful error message
-      throw BoutException("Could not convert {:s} to type {:s}",
-                          option.str(), typeid(T).name());
-    }
+  }
+  try {
+    return set(option, value
+                           + bout::utils::variantStaticCastOrThrow<Options::ValueType, T>(
+                               option.value));
+  } catch (const std::bad_cast& e) {
+    // Convert to a more useful error message
+    throw BoutException("Could not convert {:s} to type {:s}", option.str(),
+                        typeid(T).name());
   }
 }
 
@@ -477,14 +478,15 @@ template<typename T>
 Options& subtract(Options& option, T value) {
   if (!option.isSet()) {
     return set(option, -value);
-  } else {
-    try {
-      return set(option, bout::utils::variantStaticCastOrThrow<Options::ValueType, T>(option.value) - value);
-    } catch (const std::bad_cast &e) {
-      // Convert to a more useful error message
-      throw BoutException("Could not convert {:s} to type {:s}",
-                          option.str(), typeid(T).name());
-    }
+  }
+  try {
+    return set(option,
+               bout::utils::variantStaticCastOrThrow<Options::ValueType, T>(option.value)
+                   - value);
+  } catch (const std::bad_cast& e) {
+    // Convert to a more useful error message
+    throw BoutException("Could not convert {:s} to type {:s}", option.str(),
+                        typeid(T).name());
   }
 }
 

include/component_scheduler.hxx

@@ -1,10 +1,11 @@
 #pragma once
-
 #ifndef COMPONENT_SCHEDULER_H
 #define COMPONENT_SCHEDULER_H
 
-#include <vector>
 #include <memory>
+#include <set>
+#include <string>
+#include <vector>
 
 #include <bout/bout_types.hxx>
 #include <bout/options.hxx>

src/component_scheduler.cxx

@@ -1,15 +1,23 @@
 #include <algorithm>
+#include <cstddef>
+#include <iterator>
+#include <map>
 #include <memory>
+#include <set>
 #include <string>
+#include <utility>
 #include <vector>
 
 #include <bout/bout_types.hxx>
+#include <bout/boutexception.hxx>
 #include <bout/options.hxx>
 #include <bout/utils.hxx> // for trim, strsplit
+#include <fmt/format.h>
 #include <fmt/ranges.h>
 
 #include "../include/component.hxx"
 #include "../include/component_scheduler.hxx"
+#include "../include/permissions.hxx"
 
 const std::set<std::string> ComponentScheduler::predeclared_variables = {
     "time",          "linear",      "units:inv_meters_cubed", "units:eV", "units:Tesla",
@@ -46,7 +54,8 @@ void topological_sort(const std::vector<std::set<size_t>>& dependencies, size_t
 /// separated by colons in the path.
 std::set<std::string> getParents(const std::string& name) {
   std::set<std::string> result;
-  size_t start = 0, position = name.find(":", start);
+  size_t start = 0;
+  size_t position = name.find(":", start);
   while (position != std::string::npos) {
     result.insert(name.substr(0, position));
     start = position + 1;
@@ -77,7 +86,8 @@ getVariableHierarchy(const std::vector<std::unique_ptr<Component>>& components)
   // Build up a set of all section/variable names which are definitely
   // read/written by components, and the sections which they imply
   // exist
-  std::set<std::string> unconditional_names, unconditional_sections;
+  std::set<std::string> unconditional_names;
+  std::set<std::string> unconditional_sections;
   for (const auto& component : components) {
     const Permissions& permissions = component->getPermissions();
     for (const auto& [varname, _] :
@@ -191,8 +201,9 @@ setReadDependencies(const std::vector<std::unique_ptr<Component>>& components,
     // them
     for (const auto& [name, regions] :
          permissions.getVariablesWithPermission(permission)) {
-      if (ComponentScheduler::predeclared_variables.count(name) > 0)
+      if (ComponentScheduler::predeclared_variables.count(name) > 0) {
         continue;
+      }
       for (const auto& sub_name : expandVariableName(hierarchy, permissions, name)) {
         for (const auto& [region, _] : Permissions::fundamental_regions) {
           if ((regions & region) == region) {
@@ -211,25 +222,21 @@ setReadDependencies(const std::vector<std::unique_ptr<Component>>& components,
   return missing;
 }
 
-/// Consumes a list of components and returns a new one that has been
-/// topolgically sorted to ensure variables are written and read in
-/// the right order.
-std::vector<std::unique_ptr<Component>>
-sortComponents(std::vector<std::unique_ptr<Component>>&& components) {
+/// Topologically sorts the list of components to ensure variables are
+/// written and read in the right order.
+void sortComponents(std::vector<std::unique_ptr<Component>>& components) {
   // Map between variable/section names specified by component
   // permissions and the variables they contain. In the case of
   // sections this is all variables within the section and any
   // sub-sections. Non-section viarables map to themselves.
-  std::map<std::string, std::set<std::string>> variable_hierarchy =
+  const std::map<std::string, std::set<std::string>> variable_hierarchy =
       getVariableHierarchy(components);
 
   // Get information on which components write each variable
-  std::map<Var, std::set<size_t>> nonfinal_writes = getPermissionComponentMap(
-                                      components, variable_hierarchy,
-                                      PermissionTypes::Write),
-                                  final_writes = getPermissionComponentMap(
-                                      components, variable_hierarchy,
-                                      PermissionTypes::Final);
+  std::map<Var, std::set<size_t>> nonfinal_writes =
+      getPermissionComponentMap(components, variable_hierarchy, PermissionTypes::Write);
+  std::map<Var, std::set<size_t>> final_writes =
+      getPermissionComponentMap(components, variable_hierarchy, PermissionTypes::Final);
 
   // Object mapping between components (reprsented by the index of
   // that component in the `components` argument) and the components
@@ -244,7 +251,7 @@ sortComponents(std::vector<std::unique_ptr<Component>>&& components) {
       throw BoutException(
           "Multiple components have permission to make final write to variable {}", var);
     }
-    for (size_t i : comp_indices) {
+    for (const size_t i : comp_indices) {
       const auto item = nonfinal_writes.find(var);
       if (item != nonfinal_writes.end()) {
         // Note that calling merge actually removes the items from the
@@ -283,8 +290,8 @@ sortComponents(std::vector<std::unique_ptr<Component>>&& components) {
                       PermissionTypes::ReadIfSet, component_dependencies);
 
   // Create ancillary variables for sorting process
-  std::vector<bool> processing(components.size(), false),
-      processed(components.size(), false);
+  std::vector<bool> processing(components.size(), false);
+  std::vector<bool> processed(components.size(), false);
   std::vector<std::size_t> order;
 
   // Perform the sort
@@ -300,7 +307,7 @@ sortComponents(std::vector<std::unique_ptr<Component>>&& components) {
     std::swap(result[i], components[order[i]]);
   }
 
-  return result;
+  components = std::move(result);
 }
 
 ComponentScheduler::ComponentScheduler(Options &scheduler_options,
@@ -372,7 +379,7 @@ ComponentScheduler::ComponentScheduler(Options &scheduler_options,
     component->declareAllSpecies(species);
   }
 
-  components = sortComponents(std::move(components));
+  sortComponents(components);
 }
 
 std::unique_ptr<ComponentScheduler> ComponentScheduler::create(Options &scheduler_options,