Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Finalizer class #684

Closed
wants to merge 12 commits into from
Closed

Finalizer class #684

wants to merge 12 commits into from

Conversation

timspainNERSC
Copy link
Collaborator

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 timspainNERSC added the enhancement New feature or request label Sep 4, 2024
@timspainNERSC timspainNERSC added this to the 3 Stand-alone model milestone Sep 4, 2024
@timspainNERSC timspainNERSC self-assigned this Sep 4, 2024
@timspainNERSC timspainNERSC marked this pull request as ready for review September 4, 2024 15:54
Copy link
Member

@Thanduriel Thanduriel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
  • 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.

@timspainNERSC
Copy link
Collaborator Author

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.
  • I have converted to using here and elsewhere as I touch the files. And yes, the typedef syntax is always confusing!
  • Not register! That's a C reserved word. I went with registerFunc and registerUnique.
  • Okay, sure.
  • Removed run() I was using it for testing and kept it in case I though it would be useful. But what you say makes sense.
  • Perhaps regular, old function pointers would be enough.

@Thanduriel
Copy link
Member

* 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
Copy link
Collaborator Author

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

@timspainNERSC
Copy link
Collaborator Author

Merged as part of #674

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

Finalize classes and objects without using std::atexit
2 participants