Skip to content

Commit

Permalink
Merge pull request #2861 from boutproject/next-options-nocopy
Browse files Browse the repository at this point in the history 
Options: Delete copy constructor and copy assignment
  • Loading branch information
@bendudson
bendudson committed Feb 16, 2024
2 parents 4936d5e + cc48b1b commit 3a29bcd
Show file tree
Hide file tree
Showing 10 changed files with 144 additions and 124 deletions.
100 changes: 80 additions & 20 deletions include/bout/options.hxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@
* options and allows access to all sub-sections
*
**************************************************************************
* Copyright 2010 B.D.Dudson, S.Farley, M.V.Umansky, X.Q.Xu
* Copyright 2010-2024 BOUT++ contributors
*
* Contact: Ben Dudson, bd512@york.ac.uk
* Contact: Ben Dudson, dudson2@llnl.gov
*
* This file is part of BOUT++.
*
Expand All @@ -36,8 +36,8 @@
class Options;

#pragma once
#ifndef __OPTIONS_H__
#define __OPTIONS_H__
#ifndef OPTIONS_H
#define OPTIONS_H

#include "bout/bout_types.hxx"
#include "bout/field2d.hxx"
Expand Down Expand Up @@ -155,6 +155,30 @@ class Options;
* This is used to represent all the options passed to BOUT++ either in a file or on the
* command line.
*
* Copying options
* ---------------
*
* The copy constructor and copy assignment operator are deleted, so
* this is a compile-time error:
*
* Options options2 = options1["value"];
*
* This is because it's ambiguous what is meant, and because accidental copies
* were a frequent source of hard-to-understand bugs. Usually a reference is
* intended, rather than a copy:
*
* Options& ref = options1["value"];
*
* so that changes to `ref` or its children are reflected in `options1`.
* If the intent is to copy the value of the option, then just copy that:
*
* option2.value = options1["value"].value;
*
* If a full deep copy of the option, its attributes and children
* (recursively) is really desired, then use the `copy()` method:
*
* Options options2 = options1["value"].copy();
*
*/
class Options {
public:
Expand All @@ -175,22 +199,63 @@ public:
assign<T>(value);
}

/// The type used to store values
using ValueType =
bout::utils::variant<bool, int, BoutReal, std::string, Field2D, Field3D, FieldPerp,
Array<BoutReal>, Matrix<BoutReal>, Tensor<BoutReal>>;

/// A tree representation with leaves containing ValueType.
/// Used to construct Options from initializer lists.
///
/// Note: Either there are children OR value is assigned
struct CopyableOptions {
template <typename T>
CopyableOptions(T value) : value(std::move(value)) {}

/// Special case for char*, which can otherwise become cast to bool
CopyableOptions(const char* value) : value(std::string(value)) {}

CopyableOptions(
std::initializer_list<std::pair<std::string, CopyableOptions>> children)
: children(children) {}
ValueType value;
std::initializer_list<std::pair<std::string, CopyableOptions>> children;
};

/// Type of initializer_list that can be used to create Options
/// This is a workaround for initializer_lists not being movable.
using InitializerList = std::initializer_list<std::pair<std::string, CopyableOptions>>;

/// Construct with a nested initializer list
/// This allows Options trees to be constructed, using a mix of types.
/// This allows Options trees to be constructed using a mix of types.
///
/// Example: { {"key1", 42}, {"key2", field} }
Options(std::initializer_list<std::pair<std::string, Options>> values);
///
/// Note: Options doesn't have a copy constructor, and initializer lists
/// don't play nicely with uncopyable types. Instead, we create
/// a tree of CopyableOptions and then move.
Options(InitializerList values, Options* parent_instance = nullptr,
std::string section_name = "");

Options(const Options& other);
/// Options must be explicitly copied
///
/// Option option2 = option1.copy();
///
Options(const Options& other) = delete; // Use a reference or .copy() method

/// Copy assignment
/// Copy assignment must be explicit
///
/// This replaces the value, attributes and all children
/// Option option2 = option1.copy();
///
/// Note that if only the value is desired, then that can be copied using
/// the value member directly e.g. option2.value = option1.value;
/// Note that the value can be copied using:
///
Options& operator=(const Options& other);
/// option2.value = option1.value;
///
Options& operator=(const Options& other) = delete; // Use a reference or .copy() method

/// Make a deep copy of this Options,
/// recursively copying children.
Options copy() const;

Options(Options&& other) noexcept;
Options& operator=(Options&& other) noexcept;
Expand All @@ -203,11 +268,6 @@ public:
/// Free all memory
static void cleanup();

/// The type used to store values
using ValueType =
bout::utils::variant<bool, int, BoutReal, std::string, Field2D, Field3D, FieldPerp,
Array<BoutReal>, Matrix<BoutReal>, Tensor<BoutReal>>;

/// The type used to store attributes
/// Extends the variant class so that cast operator can be implemented
/// and assignment operator overloaded
Expand Down Expand Up @@ -538,8 +598,8 @@ public:
ASSERT0(def.isValue());

if (is_section) {
// Option not found
*this = def;
// Option not found. Copy the value from the default.
this->_set_no_check(def.value, DEFAULT_SOURCE);

output_info << _("\tOption ") << full_name << " = " << def.full_name << " ("
<< DEFAULT_SOURCE << ")\n";
Expand Down Expand Up @@ -1013,4 +1073,4 @@ struct fmt::formatter<Options> : public bout::details::OptionsFormatterBase {};

// NOLINTEND(cppcoreguidelines-macro-usage)

#endif // __OPTIONS_H__
#endif // OPTIONS_H
3 changes: 1 addition & 2 deletions include/bout/options_io.hxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,7 @@ public:
/// {"type", "netcdf"},
/// {"append", false}
/// });
static std::unique_ptr<OptionsIO>
create(std::initializer_list<std::pair<std::string, Options>> config_list) {
static std::unique_ptr<OptionsIO> create(Options::InitializerList config_list) {
Options config(config_list); // Construct an Options to pass by reference
return create(config);
}
Expand Down
14 changes: 6 additions & 8 deletions src/mesh/data/gridfromfile.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ bool GridFile::getField(Mesh* m, T& var, const std::string& name, BoutReal def,
return false;
}

Options option = data[name];
Options& option = data[name];

// Global (x, y, z) dimensions of field
const std::vector<int> size = bout::utils::visit(GetDimensions{}, option.value);
Expand Down Expand Up @@ -499,8 +499,7 @@ bool GridFile::get(Mesh* UNUSED(m), std::vector<BoutReal>& var, const std::strin
bool GridFile::hasXBoundaryGuards(Mesh* m) {
// Global (x,y) dimensions of some field
// a grid file should always contain "dx"
Options option = data["dx"];
const std::vector<int> size = bout::utils::visit(GetDimensions{}, option.value);
const std::vector<int> size = bout::utils::visit(GetDimensions{}, data["dx"].value);

if (size.empty()) {
// handle case where "dx" is not present - non-standard grid file
Expand Down Expand Up @@ -535,8 +534,7 @@ bool GridFile::readgrid_3dvar_fft(Mesh* m, const std::string& name, int yread, i
}

/// Check the size of the data
Options option = data[name];
const std::vector<int> size = bout::utils::visit(GetDimensions{}, option.value);
const std::vector<int> size = bout::utils::visit(GetDimensions{}, data[name].value);

if (size.size() != 3) {
output_warn.write("\tWARNING: Number of dimensions of {:s} incorrect\n", name);
Expand Down Expand Up @@ -622,7 +620,7 @@ bool GridFile::readgrid_3dvar_real(const std::string& name, int yread, int ydest
return false;
}

Options option = data[name];
Options& option = data[name];

/// Check the size of the data
const std::vector<int> size = bout::utils::visit(GetDimensions{}, option.value);
Expand Down Expand Up @@ -665,7 +663,7 @@ bool GridFile::readgrid_perpvar_fft(Mesh* m, const std::string& name, int xread,
}

/// Check the size of the data
Options option = data[name];
Options& option = data[name];
const std::vector<int> size = bout::utils::visit(GetDimensions{}, option.value);

if (size.size() != 2) {
Expand Down Expand Up @@ -748,7 +746,7 @@ bool GridFile::readgrid_perpvar_real(const std::string& name, int xread, int xde
}

/// Check the size of the data
Options option = data[name];
Options& option = data[name];
const std::vector<int> size = bout::utils::visit(GetDimensions{}, option.value);

if (size.size() != 2) {
Expand Down
2 changes: 1 addition & 1 deletion src/solver/impls/arkode/arkode.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ int ArkodeSolver::init() {
f2dtols.reserve(f2d.size());
std::transform(begin(f2d), end(f2d), std::back_inserter(f2dtols),
[abstol = abstol](const VarStr<Field2D>& f2) {
auto f2_options = Options::root()[f2.name];
auto& f2_options = Options::root()[f2.name];
const auto wrong_name = f2_options.isSet("abstol");
if (wrong_name) {
output_warn << "WARNING: Option 'abstol' for field " << f2.name
Expand Down
4 changes: 2 additions & 2 deletions src/solver/impls/cvode/cvode.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ int CvodeSolver::init() {
f2dtols.reserve(f2d.size());
std::transform(begin(f2d), end(f2d), std::back_inserter(f2dtols),
[this](const VarStr<Field2D>& f2) {
auto f2_options = Options::root()[f2.name];
auto& f2_options = Options::root()[f2.name];
const auto wrong_name = f2_options.isSet("abstol");
if (wrong_name) {
output_warn << "WARNING: Option 'abstol' for field " << f2.name
Expand Down Expand Up @@ -485,7 +485,7 @@ CvodeSolver::create_constraints(const std::vector<VarStr<FieldType>>& fields) {
constraints.reserve(fields.size());
std::transform(begin(fields), end(fields), std::back_inserter(constraints),
[](const VarStr<FieldType>& f) {
auto f_options = Options::root()[f.name];
auto& f_options = Options::root()[f.name];
const auto value =
f_options["positivity_constraint"]
.doc(fmt::format("Constraint to apply to {} if "
Expand Down
107 changes: 35 additions & 72 deletions src/sys/options.cxx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@

#include <algorithm>
#include <cmath>
#include <initializer_list>
#include <iterator>
#include <map>
#include <set>
Expand All @@ -46,17 +45,23 @@ Options& Options::root() {

void Options::cleanup() { root() = Options{}; }

Options::Options(const Options& other)
: value(other.value), attributes(other.attributes),
parent_instance(other.parent_instance), full_name(other.full_name),
is_section(other.is_section), children(other.children),
value_used(other.value_used) {
Options Options::copy() const {
Options result;

// Ensure that this is the parent of all children,
// otherwise will point to the original Options instance
for (auto& child : children) {
child.second.parent_instance = this;
result.value = value;
result.attributes = attributes;
result.parent_instance = parent_instance;
result.full_name = full_name;
result.is_section = is_section;
result.value_used = value_used;

// Recursively copy children
for (const auto& child_it : children) {
auto pair_it = result.children.emplace(child_it.first, child_it.second.copy());
Options& child = pair_it.first->second;
child.parent_instance = &result;
}
return result;
}

Options::Options(Options&& other) noexcept
Expand All @@ -77,39 +82,25 @@ Options::Options(const char* value) {
assign<std::string>(value);
}

Options::Options(std::initializer_list<std::pair<std::string, Options>> values) {
// Yes, this looks bad, but bear with me... The _only_ way to
// construct a nested initializer_list is inside-out, from the
// bottom of the tree structure. Unfortunately, this is very much
// not how you would want to construct the tree of options, as we
// don't have the parent section's name as we construct each
// child. Therefore, when we _do_ construct the parent, we'll need
// to recursively step down the tree, prepending the parent's name
// to each child. Rather than have a private member to do that, we
// use a lambda. And to make that lambda recursive, we need to have
// a nested lambda.
auto append_section_name = [](auto& children, const std::string& section_name) {
auto append_impl = [](auto& children, const std::string& section_name,
auto& append_ref) mutable -> void {
for (auto& child : children) {
child.second.full_name =
fmt::format("{}:{}", section_name, child.second.full_name);
if (child.second.is_section) {
append_ref(child.second.children, section_name, append_ref);
}
}
};
append_impl(children, section_name, append_impl);
};

for (const auto& value : values) {
(*this)[value.first] = value.second;
// value.second was constructed from the "bare" `Options<T>(T)` so
// doesn't have `full_name` set. This clobbers
// `(*this)[value.first].full_name` in the copy constructor, so we
// need to explicitly set it again
(*this)[value.first].full_name = value.first;
append_section_name((*this)[value.first].children, value.first);
Options::Options(InitializerList values, Options* parent_instance,
std::string section_name)
: parent_instance(parent_instance), full_name(std::move(section_name)),
is_section(true) {
for (const auto& value_it : values) {
std::string child_name = full_name.empty()
? value_it.first
: fmt::format("{}:{}", full_name, value_it.first);
if (value_it.second.children.size() != 0) {
// A section, so construct with an initializer_list
children.emplace(value_it.first,
Options(value_it.second.children, this, std::move(child_name)));
} else {
// A value
auto pair_it =
children.emplace(value_it.first, Options(this, std::move(child_name)));
Options& child = pair_it.first->second;
child._set_no_check(value_it.second.value, "");
}
}
}

Expand Down Expand Up @@ -230,34 +221,6 @@ Options::fuzzyFind(const std::string& name, std::string::size_type distance) con
return matches;
}

Options& Options::operator=(const Options& other) {
if (this == &other) {
return *this;
}

// Note: Here can't do copy-and-swap because pointers to parents are stored

value = other.value;

// Assigning the attributes.
// The simple assignment operator fails to compile with Apple Clang 12
// attributes = other.attributes;
attributes.clear();
attributes.insert(other.attributes.begin(), other.attributes.end());

full_name = other.full_name;
is_section = other.is_section;
children = other.children;
value_used = other.value_used;

// Ensure that this is the parent of all children,
// otherwise will point to the original Options instance
for (auto& child : children) {
child.second.parent_instance = this;
}
return *this;
}

Options& Options::operator=(Options&& other) noexcept {
if (this == &other) {
return *this;
Expand Down Expand Up @@ -833,7 +796,7 @@ Options Options::getUnused(const std::vector<std::string>& exclude_sources) cons
// Copy this object, and then we're going to chuck out everything
// that has been used. This turns out to be easier than copying just
// the unused options into an empty instance
Options unused = *this;
Options unused = this->copy();

if (unused.isValue()) {
// If this is from an excluded source, count it as being used
Expand Down

0 comments on commit 3a29bcd

Please sign in to comment.