Simplify code and avoid dereferencing null pointer

[gassmoeller]

While looking at #6556 I noticed that the code of the reactive fluid transport model is a bit confusing and in particular would potentially dereference a null pointer (if out is a null pointer the old line 89 would dereference it).

I fixed the null pointer dereference, but I still have an open question about the functionality of the function: In the current implementation this function will not do anything if out is not provided, even for the options that do not require access to out. This seems counterintuitive to the idea of an optional argument, either we should assert that out is always provided (if we only want to compute melt fractions if it exists), or we should support the case to compute melt fractions if possible even if out is not provided. Making an argument optional, but then disabling the whole function if it is not provided just seems like an unintuitive middle ground. I am happy to make either change, but am unsure which way is the correct way to go.

@danieldouglas92 any thoughts on this?

[tjhei]

Not sure, but this code looks like it requires out to be non-null in all cases (even if the current version references the nullptr first). Maybe require out to be non-null and see if anything breaks?

[gassmoeller]

What do you think about this version (review is easier if you hide the white space)? I think this is pretty good, because:

1. It only checks for the existence of the optional variables in the cases that actually need them.
2. It removes the awkward check for existence of the optional variables in the outer if-block. This means now the melt_fraction function always does something, it either computes the correct values, or it crashes. Previously there was this weird possibility that the function would be called, but wouldnt do anything.
3. I introduced an Assert outside the function in evaluate that ensured fluid_out exists, because we use it outside the function as well (without ensuring it exists).

Let's see what the testers say.

[tjhei]

This looks more sensible for sure, but we repeatedly check and grab the additional outputs in the loop this way and the inner logic is also mostly duplicated, right? What do you think about something like

if (katz2003)
{
for (q ...)
  melt_fractions[q] = katz2003_model.melt_fraction(..);
}
else
{
  Assert(out != nullptr,
                       ExcMessage("The material model 'ReactiveFluidTransport' requires the material model "
                                  "outputs in order to compute melt fractions, but none were provided."));
  Assert(out->template has_additional_output_object<MeltOutputs<dim>>(),
                       ExcMessage("The material model 'ReactiveFluidTransport' requires melt material "
                                  "outputs to compute the melt fractions, but none were provided."));

  const std::shared_ptr<const MeltOutputs<dim>> fluid_out = out->template get_additional_output_object<MeltOutputs<dim>>();
  for (q)
  {
                const double volume_fraction_porosity = in.composition[q][porosity_idx];
                const double bulk_density = compute_bulk_density(volume_fraction_porosity, out->densities[q], fluid_out->fluid_densities[q]);
                const double mass_fraction_porosity = compute_mass_fraction(volume_fraction_porosity, fluid_out->fluid_densities[q], bulk_density);

    switch (...)
    {
    }
}

[gassmoeller]

I had been thinking about that myself as well but originally decided against it. I changed it to your suggestion.

[tjhei]

very clean, now. 👍