nextsimhub / nextsimdg

neXtSIM_DG : next generation sea-ice model with DG
https://nextsim-dg.readthedocs.io/en/latest/?badge=latest
Apache License 2.0
10 stars 13 forks source link

Finalizer class #684

Closed timspainNERSC closed 1 month ago

timspainNERSC commented 2 months ago

Finalizer class

Fixes #683


Change Description

Adds a finalization class Finalizer which is designed to mimic std::atexit(), but with more flexibility. The class has a static function finalize(), which will execute all of the functions designated to be executed. As with std::atexit(), the functions must have a signature void().

The class also allows the finalization functions to be cleared, to be queried by a contains(fn) function, counted and also run without clearing the functions.


Test Description

A test is included in this PR.


Documentation Impact

The class functions have Doxygen comments describing them.

timspainNERSC commented 2 months ago

All in all it looks fine to me for its purpose but I have a few nitpicks:

* Prefer `using` instead of `typedef`. This is something I have noticed throughout the code base and is a matter of taste but the consensus in the C++ community is to always use `using` since the syntax is more intuitive and it is more powerful (cf. the [coreguidelines](http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rt-using))

* The names `atfinal` and `atfinalUnique` are not ideal. They are not verbs and not properly in camelCase. I suggest `register` and `registerUnique` instead.

* I don't see a reason why the functions need to be implemented `inline` in the header. Can you move the methods into a source file?

* I am not entirely sure about the `run` functionality. I would not expect a destructor-like function to be valid if called twice. So providing this functionally seems like a recipe for bugs. Similarly `clear` looks like an invitation for resource leaks but since the philosophy in the project so far has been "the OS is going to clean up in the end" instead of careful lifetime management, such functionality might be useful.

* There are some unnecessary copies of somewhat expensive `std::function` objects but the Finalizer should not be performance critical so it does't really matter.
Thanduriel commented 2 months ago
* Perhaps regular, old function pointers would be enough.

I think std::function is the right choice. The way to prevent copies i was thinking about was to introduce perfect forwarding and move semantics:

template<typename Fn>
static void registrFn(Fn&& fn)
{
    auto& fns = functions();  
    fns.emplace_back(std::forward<Fn>(fn));  
}

Or with slightly less potential for confusing error messages:

static void registrFn(FinalFn fn)
{
    auto& fns = functions();  
    fns.emplace_back(std::move(fn));  
}

But the added complexity is probably not worth the performance gain here. The second location where to my mind unnecessary copies occur are in finalize(). There only the order would need to be changed, i.e. call the function from a reference and then pop the entry from the list.

Really, it is best to just ignore my comments on performance. We can always do these optimizations later if it ever becomes an issue. Lets just merge this once the other changes are done.

timspainNERSC commented 2 months ago

My design was to mimic std::atexit(), which only accepts regular functions (I think?), so only accepting function pointers is enough.

timspainNERSC commented 1 month ago

Merged as part of #674