geodynamics / aspect

A parallel, extensible finite element code to simulate convection in both 2D and 3D models.
https://aspect.geodynamics.org/
Other
224 stars 235 forks source link

Avoid bare pointers. #5668

Open bangerth opened 3 months ago

bangerth commented 3 months ago

We have a number of places where we return bar pointers by functions:

include/aspect/material_model/interface.h:453:      AdditionalInputType *get_additional_input();
include/aspect/material_model/interface.h:459:      const AdditionalInputType *get_additional_input() const;
include/aspect/material_model/interface.h:627:      AdditionalOutputType *get_additional_output();
include/aspect/material_model/interface.h:633:      const AdditionalOutputType *get_additional_output() const;

I think we should avoid bare pointers. Internally, these classes store these objects as std::unique_ptrs. A better scheme would be if we store std::shared_ptr objects and return std::weak_ptrs from these functions.

Opinions?

gassmoeller commented 3 months ago

I agree the raw pointers are leftovers from a long time ago. In other places we have replaced the raw pointers with a function that checks if an input/output/plugin exists, and one that returns a reference to the specific type, e.g. the processors have:

        /**
         * Go through the list of all postprocessors that have been selected
         * in the input file (and are consequently currently active) and return
         * true if one of them has the desired type specified by the template
         * argument.
         *
         * This function can only be called if the given template type (the first template
         * argument) is a class derived from the Interface class in this namespace.
         */
        template <typename PostprocessorType,
                  typename = typename std::enable_if_t<std::is_base_of<Interface<dim>,PostprocessorType>::value>>
        bool
        has_matching_postprocessor () const;

        /**
         * Go through the list of all postprocessors that have been selected
         * in the input file (and are consequently currently active) and see
         * if one of them has the type specified by the template
         * argument or can be casted to that type. If so, return a reference
         * to it. If no postprocessor is active that matches the given type,
         * throw an exception.
         *
         * This function can only be called if the given template type (the first template
         * argument) is a class derived from the Interface class in this namespace.
         */
        template <typename PostprocessorType,
                  typename = typename std::enable_if_t<std::is_base_of<Interface<dim>,PostprocessorType>::value>>
        const PostprocessorType &
        get_matching_postprocessor () const;

I dont care much either way (weak_ptrs or references), the form with references has the advantage that it already exists in several plugin systems, but if there are reasons for using the pointers for the material model that is fine with me.

bangerth commented 3 months ago

I poked around a bit to see whether we could switch to references. For the additional inputs that wouldn't be too hard. For the additional outputs, it might be more difficult. I might poke around this a bit over the coming days.

tjhei commented 3 months ago

or std::optional...

bangerth commented 3 months ago

I thought about that too, but you can't have std::optional<T&> and we don't want to return these objects by value via std::optional<T>.