aspect icon indicating copy to clipboard operation
aspect copied to clipboard

Initialization order is error-prone.

Open bangerth opened this issue 2 years ago • 2 comments

In working on #4889, I realized something that had previously occurred to me: When we create the various plugin systems in the constructor of Simulator, we create some of the variables in the member variable initializer list and others in the body of the constructor. For the latter, we generally follow logic such as

    for (const auto &p : parameters.prescribed_traction_boundary_indicators)
      {
        BoundaryTraction::Interface<dim> *bv
          = BoundaryTraction::create_boundary_traction<dim>
            (p.second.second);
        boundary_traction[p.first].reset (bv);
        if (SimulatorAccess<dim> *sim = dynamic_cast<SimulatorAccess<dim>*>(bv))
          sim->initialize_simulator(*this);
        bv->parse_parameters (prm);
        bv->initialize ();
      }

This is awkward (and cost me ~2 hours of debugging in #4891) because during initialization of some objects, we are accessing other objects via the SimulatorAccess class. A better design ensures that we first create all objects in a first phase, and then initialize all in a second phase. I believe that we should split that second phase into calling parse_parameters() for all objects first, and then call initialize() later -- so, three phases.

bangerth avatar Jul 11 '22 22:07 bangerth

Makes sense to me.

tjhei avatar Jul 12 '22 01:07 tjhei

I think this is reasonable. One thing you need to be careful with is that the order of the plugin systems remains exactly the same. The current order is the result of a lot of reshuffling until all dependencies are satisfied. I hope none of the plugins require initialized information inside parse_parameters (but if they do the tests hopefully catch it, and that should be fixed anyway).

gassmoeller avatar Jul 13 '22 19:07 gassmoeller