Closed timspainNERSC closed 1 month ago
Thanks @timspainNERSC , I have approved from my side. I left two conversations "unresolved" just so we can find them easily in the next team meeting and discuss. No outstanding issues. Thanks for addressing my comments
For the big picture of resource management, a possible pitfall which remains is that modules have to be manually registered for destruction via
Module::finalize
. That seems manageable but is there a reason why this should be done manually instead of automatically, e.g. on construction?
I thought that there were cases where registering the final methods at construction would not work. But I cannot identify any right now. This might be worth looking at in the (not to distant) future, but can be a separate issue.
Resolving static lifetime problems
Fixes #614
Change Description
Deleted classes
ScopedTimer
Timer
Edited classes
ConfigMap
Changed the
static const
type constants to anenum
.Configurator
Made the static data into singletonoid functions. The data in question are:
AdditionalConfiguration
objectThe command line values were combined into a struct to avoid passing more than one return value. The pointer to the additional configuration was set to a
nullptr
initially, with null checks used when the pointer value is used.Configured
class templateNothing depends on the keyMap template member variable being a template member variable. Redefine each key map as
static
(compilation unit localized)const std::map
. Since many classes are configured, this affects many files. Only the source.cpp
files are affected.ConfiguredModule
&Module
Changes to
Module<>
Module
independent of Nextsim. It is now a separate Module base class that can be used for any C++ project.NextsimModule
class that specializes the newly genericModule
for nextSIM-DG.#include "Module.hpp"
with#include "NextsimModule.hpp"
.Module
namespace alongside theModule
template class. This string-to-string map is not available elsewhere. The data is stored as a function-localstatic
variable.Module<>
template. This static instance is now a function-localstatic
variable, so at least the construction order is well defined. The lifetime is still until the executable epilogue.DiagnosticOutputModule
class that inherited fromModule<IDiagnosticModule>
, everything now refers to the template instantiation. The…Module
class was necessary to run the register the module at static start-up. With this no longer necessary,…Module.hpp
) have been removedmodule_header.py
has been removedCMakeLists.txt
to run the header script has been removed.#include
s of the…Module.hpp
file have been replaced with an#include
ofNextsimModule.hpp
and an#include
of the interface header file.Module<>
. It now depends on the definition of two classes before it is#include
d:HelpMap
Config
which defines astd::string Config::getImpl(const std::string&)
function.Module<>
to be function-local statics. This requires a slight change to the API and a different format in the generatedmodule.cpp
files.fn
type is now a plain function pointer, rather than astd::function
object. This required replacing a lambda with an ad-hoc function in one test.namespace Module::InterfaceClass
.module_builder.py
to support the above changes.Module<>
template. This will call the destructor on the static instance, allowing all the data arrays the static instance holds to be deleted.Changes to
ConfiguredModule
ConfiguredModule
. Module configurations are now parsed on demand.main
to parse the configuration for the modules.setImplementation()
.ConfiguredModule_test
now only checks that the module configuration options are parsed correctly. Everything else is handeled by theModule<>
template class.Miscellaneous other changes
ModuleLoaderTestModules
. These are very defunct.IceGrowth_test
. This is so far from the Module changes, I don't see how it could have been working before.FileCallbackCloser
Replaced the static
std::list
with a singleton function providing astd::list
.Iterator
Removed the static implementation of
NullIterator
.MissingData
Replaced the
static double
missing data value with a singleton, inlined access function. The missing data value is created on first access, but still destroyed at the end of execution. This is fine, since it is a scalar double.Removed the
MissingData.cpp
source file as it is no longer needed.ModelComponent
registerModule
and the data members it uses. Also removehField()
and similar functions, which don't now do anything, and were originally planned to be used alongside the module register.ModelArrayRefStore
to be lazily allocated.ModelConfig
Moved the key specification to an
.ipp
file to avoid repeating the text inModel.cpp
.ParaGridIO
ModelArray
equivalents toconst
instance variables that are initialized as part of the ctor.Finalizer
class to to whatstd::atexit
does, except at our convenience, rather than executable clean-up. This includes a new test for the new class. This change is the same as that contained in PR #683New classes
Finalizer
Finalize
class, which mimicsstd::atexit()
, but can be called before the end ofmain()
.Finalizer.cpp
source file to the CMake system.Finalizer
class.Module::finalize<>
function template.registerUnique()
each module instantiation is only called once when the finalization occurs.Test Description
Apart from deleted tests for deleted classes, all tests should still pass.
ModelComponent
Adds some test to
ModelArray_test
to check sizes around assignment operations.Finalizer
There is a test for the new
Finalizer
class.Documentation Impact
No changes to existing documentation. All classes should provided the same interfaces as previously.