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

Encapsulate auto-update accumulators #4821

Closed
jngrad opened this issue Nov 21, 2023 · 2 comments · Fixed by #4950
Closed

Encapsulate auto-update accumulators #4821

jngrad opened this issue Nov 21, 2023 · 2 comments · Fixed by #4950
Assignees
Labels

Comments

@jngrad
Copy link
Member

jngrad commented Nov 21, 2023

Accumulators registered via system.auto_update_accumulators are currently stored in a global variable in the C++ core. To help with multi-system simulations, the auto-update accumulators container needs to become a member of the core System class.

Procedure:

  1. get familiar with the design patterns used in the core System class: opaque pointers, composite pattern (chapter System class design in the Doxygen documentation)
  2. create an Accumulators::AutoUpdateAccumulators class
    1. rewrite the functions in src/core/accumulators.hpp as methods of class Accumulators::AutoUpdateAccumulators
      • rename the auto_update() function to an operator()() method
      • rename all auto_update_*() functions to *() methods
      • rename all auto_update_*() function calls to auto_update.*() function calls
    2. add an extern Accumulators auto_update; in src/core/accumulators.hpp
    3. add an Accumulators auto_update = {}; in src/core/accumulators.cpp
    4. make the auto_update_accumulators static global a member of the Accumulators class
    5. make all necessary adjustments until the C++ compiler is happy
    6. the testsuite should pass
  3. rewrite src/script_interface/accumulators/AutoUpdateAccumulators.hpp as a leaf container class
    1. the class should publicly inherit from ObjectList<AccumulatorBase, AutoParameters<ObjectList<AccumulatorBase, System::Leaf>, System::Leaf>>
    2. the class should manage a std::shared_ptr<::Accumulators::AutoUpdateAccumulators> m_handle; that gets initialized inside on_bind_system() via a m_params handle that was initialized inside do_construct() (use src/script_interface/electrostatics/Container.hpp as a guide)
    3. the testsuite should pass
  4. move the auto_update global to a member of the core System class
    1. add a std::shared_ptr<Accumulators::AutoUpdateAccumulators> auto_update; to the core System class, right after bond_breakage
    2. forward-declare Accumulators::AutoUpdateAccumulators at the top of src/core/system/System.hpp
    3. remove the auto_update global variable and adapt all function calls using the -> dereference operator
    4. in the script interface, the current system's auto_update member is copied into m_handle inside on_bind_system()
@1234somesh
Copy link
Contributor

1234somesh commented Nov 21, 2023 via email

@jngrad
Copy link
Member Author

jngrad commented Apr 23, 2024

The opaque pointer technique is detailed in More C++ Idioms book, chapter Pointer to Implementation. Since we are only interested in breaking a circular dependency, we use the pointer to incomplete type method. The void pointer method is far less common, and for this specific scenario it has no advantages.

@RudolfWeeber RudolfWeeber added this to the ESPResSo 4.3.0 milestone Jul 16, 2024
@jngrad jngrad assigned jngrad and unassigned davidbbeyer Jul 22, 2024
@kodiakhq kodiakhq bot closed this as completed in #4950 Aug 1, 2024
@kodiakhq kodiakhq bot closed this as completed in 4884c35 Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants