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

Resolving static lifetime problems #674

Merged
merged 105 commits into from
Oct 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
105 commits
Select commit Hold shift + click to select a range
6625cc2
Remove the unused (and thread unsafe) timer classes.
timspainNERSC Sep 3, 2024
78756b1
Remove timer classes. (#673)
timspainNERSC Sep 3, 2024
9ee9e8b
Remove the static implementaion of NullIterator.
timspainNERSC Sep 3, 2024
7914621
Remove the static implementaion of NullIterator. (#675)
timspainNERSC Sep 3, 2024
415f648
clang formatting
timspainNERSC Sep 3, 2024
510c326
clang formatting (#676)
timspainNERSC Sep 3, 2024
0cc5791
Replace static non-const MissingData with a singleton double.
timspainNERSC Sep 3, 2024
85584b3
Remove MissingData.cpp.
timspainNERSC Sep 3, 2024
e88205f
Use the default value.
timspainNERSC Sep 3, 2024
8e1f627
Change the MissingData return types.
timspainNERSC Sep 3, 2024
a0beac8
clang formatting
timspainNERSC Sep 3, 2024
44d49f3
Lazily initialize the file maps for ParaGridIO.
timspainNERSC Sep 3, 2024
09a4026
Combine the two filename based maps to ensure consistency.
timspainNERSC Sep 3, 2024
6a239f0
Add a finalizer class to mimic std::atexit, but with more control.
timspainNERSC Sep 4, 2024
ebfe38e
Better defined exception handling.
timspainNERSC Sep 4, 2024
d6c126e
clang formatting
timspainNERSC Sep 4, 2024
cbbdbe4
Merge branch 'issue683_finalizer' into issue614_ParaGridIO
timspainNERSC Sep 4, 2024
721e4bf
Use the finalizer in place of std::atexit.
timspainNERSC Sep 4, 2024
18a8aac
Make the final static maps const. Do things once for real.
timspainNERSC Sep 4, 2024
72d051c
Change the static constants to an enum.
timspainNERSC Sep 4, 2024
59945de
Change ConfigMap static constants to an enum. (#686)
timspainNERSC Sep 4, 2024
75e13d2
clang formatting (or not)
timspainNERSC Sep 4, 2024
0b64d04
Remove the static list.
timspainNERSC Sep 4, 2024
b0f9a68
Remove the FileCallbackCloser static list. (#687)
timspainNERSC Sep 4, 2024
9e0f587
Move ConfigOutput to a local config key map.
timspainNERSC Sep 5, 2024
557ca1d
Move ConfiguredAtmosphere to a local config key map.
timspainNERSC Sep 5, 2024
eb283e0
Move Model to a local config key map.
timspainNERSC Sep 5, 2024
d31c48b
Move all other classes to a local config key map.
timspainNERSC Sep 5, 2024
6677a97
Rename localKeyMap to keyMap. Fix the Configurator test.
timspainNERSC Sep 5, 2024
eddafe1
Localize Configured keyMaps (#691)
timspainNERSC Sep 5, 2024
91ebbdd
Issue614 missing data (#677)
timspainNERSC Sep 5, 2024
c988170
clang formatting
timspainNERSC Sep 5, 2024
8f438b0
Move Xios to a local config key map.
timspainNERSC Sep 5, 2024
6618588
clang formatting
timspainNERSC Sep 5, 2024
73a8359
Remove static data from Configurator.
timspainNERSC Sep 5, 2024
8a22ec7
Remove static data from Configurator. (#696)
timspainNERSC Sep 5, 2024
a854ea4
Indirectly reference the static implementation.
timspainNERSC Sep 10, 2024
ac52bca
Move Module<>::functionMap to be lazily initialized.
timspainNERSC Sep 11, 2024
c69b418
Move and rename the Module<> generationFunction.
timspainNERSC Sep 11, 2024
1f89813
Move staticInstance to be lazily constructed, but keep the old code.
timspainNERSC Sep 11, 2024
472d1f3
Simplify the Finalizer class.
timspainNERSC Sep 11, 2024
64c4525
Use function pointers, rather than heavy std::function objects.
timspainNERSC Sep 11, 2024
b438e87
Move the Finalizer functions to a cpp source file.
timspainNERSC Sep 11, 2024
4beab2e
clang formatting
timspainNERSC Sep 12, 2024
172e1a4
Clean up Finalizer.hpp
timspainNERSC Sep 12, 2024
6afdff6
Configure Modules at first request for an instance.
timspainNERSC Sep 12, 2024
53297c2
Get the configured Module implementations at acquisition.
timspainNERSC Sep 12, 2024
61753a7
Remove remaining references to parseConfigurator().
timspainNERSC Sep 12, 2024
f4e1ade
Remove redundant test classes.
timspainNERSC Sep 12, 2024
a5dc7fb
Remove the internal templates from the modules.
timspainNERSC Sep 12, 2024
6d33a6f
Don't use the Configurator to select implementations in tests.
timspainNERSC Sep 12, 2024
46a3e58
Debugging PrognosticData test.
timspainNERSC Sep 12, 2024
6fcc642
Tidy up Module.hpp.
timspainNERSC Sep 13, 2024
209e8e9
Remove the *Module.hpp files.
timspainNERSC Sep 13, 2024
32d98ed
Remove the module header python script.
timspainNERSC Sep 13, 2024
6692ad3
Prevent recursive initialization.
timspainNERSC Sep 13, 2024
8177b7c
Replace typedefs by using statements.
timspainNERSC Sep 13, 2024
2761fa7
Move the namespace function definitions to the template class.
timspainNERSC Sep 13, 2024
5c9f9d9
Make Module independent of Nextsim.
timspainNERSC Sep 13, 2024
0a52914
Move to the NextsimModule header (does not compile!).
timspainNERSC Sep 16, 2024
5e1f05b
Use NextsimModule for ConfiguredModule.
timspainNERSC Sep 16, 2024
4b936be
Revert to the Module name, remove template arguments.
timspainNERSC Sep 16, 2024
3efb2e2
Correct function visibility in Module<>.
timspainNERSC Sep 16, 2024
ae11bfa
Remove debugging statements.
timspainNERSC Sep 16, 2024
cacde72
clang formatting
timspainNERSC Sep 16, 2024
c0c1ee5
Change the implmentation name internals to avoid stale names. Test.
timspainNERSC Sep 17, 2024
ffcdc30
Namespace the implementation name strings.
timspainNERSC Sep 17, 2024
3207b58
Add getGenerationFunction to the template. Add fn for the default name.
timspainNERSC Sep 17, 2024
6f337e8
clang formatting
timspainNERSC Sep 17, 2024
07fb0b1
Add comments to the Module header.
timspainNERSC Sep 18, 2024
4c792b5
Don't throw from Module<>::implementation.
timspainNERSC Sep 18, 2024
8405eae
Suppress default initialization of a module (optionally).
timspainNERSC Sep 20, 2024
954d846
clang formatting
timspainNERSC Sep 20, 2024
b505d7d
Add a finalizer method to the Module template class.
timspainNERSC Sep 23, 2024
c0bc27c
Issue 614 Module & ConfiguredModule (#704)
timspainNERSC Sep 23, 2024
374bd7a
Revert to function objects. Remove redundant copy when finalizing.
timspainNERSC Sep 23, 2024
741e1d0
Merge branch 'issue683_finalizer' into issue614_datalifetimes
timspainNERSC Sep 23, 2024
3cc3387
Make sure to pop the throwing function.
timspainNERSC Sep 23, 2024
da9b9c2
Merge branch 'issue683_finalizer' into issue614_Modulefinalizer
timspainNERSC Sep 25, 2024
dfae6b0
Add module finalizers wherever modules are configured.
timspainNERSC Sep 30, 2024
8913b82
Fix the uniqueness detection in the finalizer.
timspainNERSC Oct 1, 2024
402fc9c
Use the finalizer after the iteration class returns.
timspainNERSC Oct 1, 2024
b0c4d35
Fix the uniqueness detection in the finalizer.
timspainNERSC Oct 1, 2024
e03f8d5
Merge branch 'issue683_finalizer' into issue614_datalifetimes
timspainNERSC Oct 1, 2024
152de35
Fix the uniqueness detection in the finalizer even further.
timspainNERSC Oct 1, 2024
9a95b9f
Fix the uniqueness detection in the finalizer even further.
timspainNERSC Oct 1, 2024
3aa8037
Module & Finalizer: the module finalizer (#709)
timspainNERSC Oct 1, 2024
9090eee
clang formatting (Model.cpp)
timspainNERSC Oct 1, 2024
c097f3e
Merge branch 'issue614_datalifetimes' into issue614_ParaGridIO
timspainNERSC Oct 1, 2024
ad65ea0
Issue 614 ParaGridIO (#685)
timspainNERSC Oct 1, 2024
8a05004
clang formatting
timspainNERSC Oct 1, 2024
3261ccf
Update the Finalizer API.
timspainNERSC Oct 1, 2024
0c84cb1
Use using statements.
timspainNERSC Oct 2, 2024
d647161
clang formatting
timspainNERSC Oct 2, 2024
620c0e1
Merge remote-tracking branch 'origin/develop' into
timspainNERSC Oct 14, 2024
b553059
Remove std::moves when passing around the implementation pointer.
timspainNERSC Oct 16, 2024
9140a77
CamelCase the Module<> convenience type names.
timspainNERSC Oct 17, 2024
d77ca1e
std::unique_ptr can be cast directly to a bool with the same result.
timspainNERSC Oct 17, 2024
a772faa
Use a reference, rather than a pointer. Delete NullIterant.
timspainNERSC Oct 17, 2024
db73a2b
clang formatting that I should have remembered.
timspainNERSC Oct 17, 2024
e478c85
Tidy up the Finalizer test.
timspainNERSC Oct 17, 2024
81fa973
Move the intialization outside the #ifdef block.
timspainNERSC Oct 17, 2024
1966366
Change the ConfiguredModule default string to "".
timspainNERSC Oct 17, 2024
ebfaf8c
Compress the doOnce functionality of ParaGridIO.
timspainNERSC Oct 17, 2024
04c6031
clang formatting
timspainNERSC Oct 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions core/src/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

set(BaseSources
"Logged.cpp"
"Timer.cpp"
"ModelConfig.cpp"
"Model.cpp"
"Iterator.cpp"
Expand All @@ -14,9 +13,9 @@ set(BaseSources
"RectGridIO.cpp"
"ParaGridIO.cpp"
"FileCallbackCloser.cpp"
"Finalizer.cpp"
"DevStep.cpp"
"StructureFactory.cpp"
"MissingData.cpp"
"ModelArray.cpp"
"ModelComponent.cpp"
"ModelMetadata.cpp"
Expand Down
8 changes: 4 additions & 4 deletions core/src/CommonRestartMetadata.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,19 +48,19 @@ netCDF::NcGroup& CommonRestartMetadata::writeRestartMetadata(

for (auto entry : metadata.m_config) {
switch (entry.second.index()) {
case (CONFIGMAP_DOUBLE): {
case (ConfigMapType::DOUBLE): {
configGroup.putAtt(entry.first, netCDF::ncDouble, *std::get_if<double>(&entry.second));
break;
}
case (CONFIGMAP_UNSIGNED): {
case (ConfigMapType::UNSIGNED): {
configGroup.putAtt(entry.first, netCDF::ncUint, *std::get_if<unsigned>(&entry.second));
break;
}
case (CONFIGMAP_INT): {
case (ConfigMapType::INT): {
configGroup.putAtt(entry.first, netCDF::ncInt, *std::get_if<int>(&entry.second));
break;
}
case (CONFIGMAP_STRING): {
case (ConfigMapType::STRING): {
std::string extring = std::get<std::string>(entry.second);
configGroup.putAtt(entry.first, std::get<std::string>(entry.second));
break;
Expand Down
20 changes: 7 additions & 13 deletions core/src/Configurator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,6 @@

namespace Nextsim {

std::vector<std::unique_ptr<std::istream>> Configurator::sources;
int Configurator::m_argc;
char** Configurator::m_argv;
static NoAdditionalConfiguration noAddConf;
Configurator::AdditionalConfiguration* Configurator::p_addConf = &noAddConf;

boost::program_options::variables_map Configurator::parse(
const boost::program_options::options_description& opt)
{
Expand All @@ -25,9 +19,9 @@ boost::program_options::variables_map Configurator::parse(
// Parse the command file for any overrides
int use_argc;
char** use_argv;
if (m_argc && m_argv) {
use_argc = m_argc;
use_argv = m_argv;
if (commandLine().m_argc && commandLine().m_argv) {
use_argc = commandLine().m_argc;
use_argv = commandLine().m_argv;
} else {
// Use a fake command line to ensure at least one parse happens in all cases
use_argc = 1;
Expand All @@ -44,7 +38,7 @@ boost::program_options::variables_map Configurator::parse(
boost::program_options::store(parsed, vm);

// Parse the named streams for configuration
for (auto iter = sources.begin(); iter != sources.end(); ++iter) {
for (auto iter = sources().begin(); iter != sources().end(); ++iter) {
try {
boost::program_options::store(
boost::program_options::parse_config_file(**iter, opt, true), vm);
Expand All @@ -66,12 +60,12 @@ void Configurator::addSStream(const std::stringstream& sstream)
addStream(std::move(std::unique_ptr<std::istream>(new std::stringstream(sstream.str()))));
}

void Configurator::setAdditionalConfiguration(AdditionalConfiguration* pAC) { p_addConf = pAC; }
void Configurator::setAdditionalConfiguration(AdditionalConfiguration* pAC) { addConf() = pAC; }

void Configurator::getAdditionalConfiguration(const std::string& source)
{
if (p_addConf) {
addSStream(p_addConf->read(source));
if (addConf()) {
addSStream(addConf()->read(source));
}
}

Expand Down
52 changes: 11 additions & 41 deletions core/src/ConfiguredModule.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
#include "include/ConfiguredModule.hpp"

#include "include/Configurator.hpp"
#include "include/Module.hpp"
#include "include/NextsimModule.hpp"

#include <boost/program_options.hpp>
#include <stdexcept>
Expand All @@ -17,59 +17,29 @@ namespace Nextsim {

const std::string ConfiguredModule::MODULE_PREFIX = "Modules";

ConfiguredModule::map ConfiguredModule::configuredModules;

void ConfiguredModule::parseConfigurator()
{
// A default string that can never be a valid C++ class name
std::string defaultStr = "+++DEFAULT+++";
// Construct a new options map
boost::program_options::options_description opt;

for (auto entry : configuredModules) {
std::string module = entry.first;
opt.add_options()(addPrefix(module).c_str(),
boost::program_options::value<std::string>()->default_value(defaultStr),
("Load an implementation of " + module).c_str());
}

boost::program_options::variables_map vm = Configurator::parse(opt);

for (auto entry : configuredModules) {
std::string implString = vm[addPrefix(entry.first)].as<std::string>();
// Only do anything if the retrieved option is not the default value
if (implString != defaultStr) {
entry.second.first(implString);
}
}
}

std::string ConfiguredModule::addPrefix(const std::string& moduleName)
{
return MODULE_PREFIX + "." + moduleName;
}

void ConfiguredModule::setConfiguredModules(const map& ls)
std::string ConfiguredModule::getImpl(const std::string& module)
{
map newMap(ls);
configuredModules.merge(newMap);
}
boost::program_options::options_description opt;

void ConfiguredModule::configureModule(const std::string& mod, const fn& setter, const ofn& getter)
{
configuredModules[mod] = { setter, getter };
}
opt.add_options()(addPrefix(module).c_str(),
boost::program_options::value<std::string>()->default_value(""),
("Load an implementation of " + module).c_str());

std::string ConfiguredModule::getModuleConfiguration(const std::string& module)
{
return configuredModules[module].second();
std::string implString = Configurator::parse(opt)[addPrefix(module)].as<std::string>();
// Only do anything if the retrieved option is not the default value
return implString;
}

ConfigMap ConfiguredModule::getAllModuleConfigurations()
{
ConfigMap iiMap;
for (auto entry : configuredModules) {
iiMap[addPrefix(entry.first)] = entry.second.second();
for (const auto& moduleImpl : Module::ImplementationNames::getAll()) {
iiMap[moduleImpl.first] = moduleImpl.second;
}
return iiMap;
}
Expand Down
4 changes: 3 additions & 1 deletion core/src/DevStep.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@
#include "include/DevStep.hpp"

#include "include/ConfiguredModule.hpp"
#include "include/Finalizer.hpp"
#include "include/IDiagnosticOutput.hpp"
#include "include/Module.hpp"
#include "include/NextsimModule.hpp"
namespace Nextsim {

DevStep::DevStep()
Expand All @@ -21,6 +22,7 @@ DevStep::DevStep()

void DevStep::init()
{
Finalizer::registerUnique(Module::finalize<IDiagnosticOutput>);
IDiagnosticOutput& ido = Module::getImplementation<IDiagnosticOutput>();
ido.setFilenamePrefix("diagnostic");
tryConfigure(ido);
Expand Down
8 changes: 3 additions & 5 deletions core/src/FileCallbackCloser.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,14 @@

namespace Nextsim {

std::list<FileCallbackCloser::ClosingFn> FileCallbackCloser::closingFns;

void FileCallbackCloser::onClose(FileCallbackCloser::ClosingFn fn) { closingFns.push_back(fn); }
void FileCallbackCloser::onClose(FileCallbackCloser::ClosingFn fn) { closingFns().push_back(fn); }

void FileCallbackCloser::close(const std::string& filename)
{
for (auto& fn : closingFns) {
for (auto& fn : closingFns()) {
fn(filename);
}
}

void FileCallbackCloser::clearAllClose() { closingFns.clear(); }
void FileCallbackCloser::clearAllClose() { closingFns().clear(); }
} /* namespace Nextsim */
61 changes: 61 additions & 0 deletions core/src/Finalizer.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*!
* @file Finalizer.cpp
*
* @date Sep 11, 2024
* @author Tim Spain <[email protected]>
*/

#include "include/Finalizer.hpp"

namespace Nextsim {

void Finalizer::registerFunc(const FinalFn& fn)
{
auto& fns = functions();
fns.push_back(fn);
}

bool Finalizer::registerUnique(const FinalFn& fn)
{
if (contains(fn))
return false;
registerFunc(fn);
return true;
}
Comment on lines +18 to +24
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason this function returns a bool. Looks like it is mostly used like a void function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The return value indicates whether the function was successfully registered (true) or not (false). I this is useful enough functionality, given the minimal cost. Even if it is not currently used.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess my concern was, if you thought you were using it but you were not. Or if it was left in for some use case that we no longer need.

Do you think it would be more useful to raise an exception if the user tries to register an already registered function? Given the use in the rest of the code, my concern is that new developers are not likely to test the boolean return value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's there as a service if the developer wants to know if the function was already registered. How it is used currently, I don't care, so I don't check. If a future developer does care, then they can check.


bool Finalizer::contains(const FinalFn& fn)
{
auto& fns = functions();
for (const auto& stored : fns) {
// Compare function addresses
if (*stored.target<FnType*>() == *fn.target<FnType*>())
return true;
}
return false;
}

void Finalizer::finalize()
{
while (!functions().empty()) {
try {
// Execute the last function in the list
functions().back()();
} catch (const std::exception& e) {
// Remove the function from the finalization list even if an exception was thrown.
functions().pop_back();
throw;
}
// Remove the function from the finalization list.
functions().pop_back();
}
}

size_t Finalizer::count() { return functions().size(); }

Finalizer::FinalFnContainer& Finalizer::functions()
{
static FinalFnContainer set;
return set;
}

}
20 changes: 3 additions & 17 deletions core/src/Iterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,6 @@

namespace Nextsim {

Iterator::NullIterant Iterator::nullIterant;

Iterator::Iterator()
: iterant(&nullIterant)
{
}

Iterator::Iterator(Iterant* iterant)
: iterant(iterant)
{
}

void Iterator::setIterant(Iterant* iterant) { this->iterant = iterant; }

void Iterator::setStartStopStep(TimePoint startTime, TimePoint stopTime, Duration timestep)
{
this->startTime = startTime;
Expand Down Expand Up @@ -53,14 +39,14 @@ TimePoint Iterator::parseAndSet(const std::string& startTimeStr, const std::stri

void Iterator::run()
{
iterant->start(startTime);
iterant.start(startTime);

for (auto t = startTime; t < stopTime; t += timestep) {
TimestepTime tsTime = { t, timestep };
iterant->iterate(tsTime);
iterant.iterate(tsTime);
}

iterant->stop(stopTime);
iterant.stop(stopTime);
}

} /* namespace Nextsim */
16 changes: 0 additions & 16 deletions core/src/MissingData.cpp

This file was deleted.

Loading
Loading