-
Notifications
You must be signed in to change notification settings - Fork 70
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
axes should provide strong exception guarantee for metadata #379
Comments
@HDembinski Here is an example of constructor rollback " #include<type_traits>
#include<iostream>
#include<boost/core/uncaught_exceptions.hpp>
template<typename T>
struct remove_rvalue_reference
{
using type = typename std::conditional_t<std::is_rvalue_reference<T>::value, std::remove_reference_t<T>, T>;
};
template<typename T>
using remove_rvalue_reference_t = typename remove_rvalue_reference<T>::type;
template<typename TSrc, typename TDst>
struct rollback_move_on_exception
{
unsigned int count_;
std::remove_reference_t<TSrc> *src_;
std::remove_reference_t<TDst> *dst_;
rollback_move_on_exception(std::remove_reference_t<TSrc> *src, std::remove_reference_t<TDst> *dst)
: count_(boost::core::uncaught_exceptions()),
src_(src),
dst_(dst)
{}
~rollback_move_on_exception()
{
if(count_ != boost::core::uncaught_exceptions())
{
*src_ = std::move(*dst_);
}
}
};
template<typename TSrc, typename TDst>
struct rollback_move_on_exception<TSrc &, TDst>
{
rollback_move_on_exception(std::remove_reference_t<TSrc> *, std::remove_reference_t<TDst> *) {}
};
#define ROLLBACK_MOVE_ON_EXCEPTION(parameter_exp, member_exp) rollback_move_on_exception<remove_rvalue_reference_t<decltype(parameter_exp)>, decltype(member_exp)> s_guard(¶meter_exp, &member_exp)
struct c1_t
{
std::string s_;
std::string&get_s(){return s_;}
int n_;
template<typename Str>
c1_t(Str &&s, bool thrw)
: s_(std::forward<Str>(s)),
n_([&]
{
ROLLBACK_MOVE_ON_EXCEPTION(s, get_s());
// ROLLBACK_MOVE_ON_EXCEPTION(s, s_);
if(thrw) throw 1;
return 0;
}())
{
ROLLBACK_MOVE_ON_EXCEPTION(s, s_);
if(thrw) throw 1;
}
};
int main(int argc, char *argv[])
{
{
std::string s("abc");
c1_t c1(s, false);
std::cout << 1 << s << '\n';
}
{
std::string s("abc");
c1_t c1(std::move(s), false);
std::cout << 2 << s << '\n';
}
{
std::string s("abc");
try
{
c1_t c1(s, true);
}
catch(...)
{
}
std::cout << 3 << s << '\n';
}
{
std::string s("abc");
try
{
c1_t c1(std::move(s), true);
}
catch(...)
{
}
std::cout << 4 << s << '\n';
}
}
good:
bad:
not possible to cover:
too troublesome to cover:
modified version of // Copyright 2015-2018 Hans Dembinski
//
// Distributed under the Boost Software License, Version 1.0.
// (See accompanying file LICENSE_1_0.txt
// or copy at http://www.boost.org/LICENSE_1_0.txt)
#ifndef BOOST_HISTOGRAM_AXIS_VARIABLE_HPP
#define BOOST_HISTOGRAM_AXIS_VARIABLE_HPP
#include <algorithm>
#include <boost/core/nvp.hpp>
#include <boost/histogram/axis/interval_view.hpp>
#include <boost/histogram/axis/iterator.hpp>
#include <boost/histogram/axis/metadata_base.hpp>
#include <boost/histogram/axis/option.hpp>
#include <boost/histogram/detail/convert_integer.hpp>
#include <boost/histogram/detail/detect.hpp>
#include <boost/histogram/detail/eytzinger_search.hpp>
#include <boost/histogram/detail/limits.hpp>
#include <boost/histogram/detail/relaxed_equal.hpp>
#include <boost/histogram/detail/replace_type.hpp>
#include <boost/histogram/fwd.hpp>
#include <boost/throw_exception.hpp>
#include <cassert>
#include <cmath>
#include <limits>
#include <memory>
#include <stdexcept>
#include <string>
#include <type_traits>
#include <utility>
#include <vector>
#include <boost/core/uncaught_exceptions.hpp>
template <typename T>
struct remove_rvalue_reference {
using type = typename std::conditional_t<std::is_rvalue_reference<T>::value,
std::remove_reference_t<T>, T>;
};
template <typename T>
using remove_rvalue_reference_t = typename remove_rvalue_reference<T>::type;
template <typename TSrc, typename TDst>
struct rollback_move_on_exception {
unsigned int count_;
std::remove_reference_t<TSrc>* src_;
std::remove_reference_t<TDst>* dst_;
rollback_move_on_exception(std::remove_reference_t<TSrc>* src,
std::remove_reference_t<TDst>* dst)
: count_(boost::core::uncaught_exceptions()), src_(src), dst_(dst) {}
~rollback_move_on_exception() {
if (count_ != boost::core::uncaught_exceptions()) { *src_ = std::move(*dst_); }
}
};
template <typename TSrc, typename TDst>
struct rollback_move_on_exception<TSrc&, TDst> {
rollback_move_on_exception(std::remove_reference_t<TSrc>*,
std::remove_reference_t<TDst>*) {}
};
#define ROLLBACK_MOVE_ON_EXCEPTION(parameter_exp, member_exp) \
rollback_move_on_exception<remove_rvalue_reference_t<decltype(parameter_exp)>, \
decltype(member_exp)> \
s_guard(¶meter_exp, &member_exp)
namespace boost {
namespace histogram {
namespace axis {
/** Axis for non-equidistant bins on the real line.
Binning is a O(log(N)) operation. If speed matters and the problem domain
allows it, prefer a regular axis, possibly with a transform.
If the axis has an overflow bin (the default), a value on the upper edge of the last
bin is put in the overflow bin. The axis range represents a semi-open interval.
If the overflow bin is deactivated, then a value on the upper edge of the last bin is
still counted towards the last bin. The axis range represents a closed interval. This
is the desired behavior for random numbers drawn from a bounded interval, which is
usually closed.
@tparam Value input value type, must be floating point.
@tparam MetaData type to store meta data.
@tparam Options see boost::histogram::axis::option.
@tparam Allocator allocator to use for dynamic memory management.
*/
template <class Value, class MetaData, class Options, class Allocator>
class variable : public iterator_mixin<variable<Value, MetaData, Options, Allocator>>,
public metadata_base_t<MetaData> {
// these must be private, so that they are not automatically inherited
using value_type = Value;
using metadata_base = metadata_base_t<MetaData>;
using metadata_type = typename metadata_base::metadata_type;
using options_type =
detail::replace_default<Options, decltype(option::underflow | option::overflow)>;
using allocator_type = Allocator;
using vector_type = std::vector<Value, allocator_type>;
public:
constexpr variable() = default;
explicit variable(allocator_type alloc) : vec_(alloc) {}
/** Construct from iterator range of bin edges.
@param begin begin of edge sequence.
@param end end of edge sequence.
@param meta description of the axis (optional).
@param options see boost::histogram::axis::option (optional).
@param alloc allocator instance to use (optional).
*/
template <class It, class Metadata = metadata_type, class = detail::requires_iterator<It>>
variable(It begin, It end, Metadata&& meta = {}, options_type options = {},
allocator_type alloc = {})
: metadata_base(std::forward<Metadata>(meta))
, vec_([&] {
ROLLBACK_MOVE_ON_EXCEPTION(meta, this->metadata());
if (std::distance(begin, end) < 2)
BOOST_THROW_EXCEPTION(std::invalid_argument("bins > 0 required"));
vector_type vec(std::move(alloc));
vec.reserve(std::distance(begin, end));
vec.emplace_back(*begin++);
bool strictly_ascending = true;
for (; begin != end; ++begin) {
strictly_ascending &= vec.back() < *begin;
vec.emplace_back(*begin);
}
if (!strictly_ascending)
BOOST_THROW_EXCEPTION(
std::invalid_argument("input sequence must be strictly ascending"));
return vec;
}())
, eytzinger_layout_and_eytzinger_binary_search_([&] {
ROLLBACK_MOVE_ON_EXCEPTION(meta, this->metadata());
return decltype(eytzinger_layout_and_eytzinger_binary_search_)(vec_);
}()) {
ROLLBACK_MOVE_ON_EXCEPTION(meta, this->metadata());
// static_asserts were moved here from class scope to satisfy deduction in gcc>=11
static_assert(
std::is_floating_point<value_type>::value,
"current version of variable axis requires floating point type; "
"if you need a variable axis with an integral type, please submit an issue");
static_assert((!options.test(option::circular) && !options.test(option::growth)) ||
(options.test(option::circular) ^ options.test(option::growth)),
"circular and growth options are mutually exclusive");
}
// kept for backward compatibility; requires_allocator is a workaround for deduction
// guides in gcc>=11
template <class It, class Metadata, class A, class = detail::requires_iterator<It>,
class = detail::requires_allocator<A>>
variable(It begin, It end, Metadata&& meta, A alloc)
: variable(begin, end, std::forward<Metadata>(meta), {}, std::move(alloc)) {}
/** Construct variable axis from iterable range of bin edges.
@param iterable iterable range of bin edges.
@param meta description of the axis (optional).
@param options see boost::histogram::axis::option (optional).
@param alloc allocator instance to use (optional).
*/
template <class U, class Metadata = metadata_type, class = detail::requires_iterable<U>>
variable(const U& iterable, Metadata&& meta = {}, options_type options = {},
allocator_type alloc = {})
: variable(std::begin(iterable), std::end(iterable), std::forward<Metadata>(meta),
options, std::move(alloc)) {}
// kept for backward compatibility; requires_allocator is a workaround for deduction
// guides in gcc>=11
template <class U, class Metadata, class A, class = detail::requires_iterable<U>,
class = detail::requires_allocator<A>>
variable(const U& iterable, Metadata&& meta, A alloc)
: variable(std::begin(iterable), std::end(iterable), std::forward<Metadata>(meta),
{}, std::move(alloc)) {}
/** Construct variable axis from initializer list of bin edges.
@param list `std::initializer_list` of bin edges.
@param meta description of the axis (optional).
@param options see boost::histogram::axis::option (optional).
@param alloc allocator instance to use (optional).
*/
template <class U, class Metadata = metadata_type>
variable(std::initializer_list<U> list, Metadata&& meta = {},
options_type options = {}, allocator_type alloc = {})
: variable(list.begin(), list.end(), std::forward<Metadata>(meta), options,
std::move(alloc)) {}
// kept for backward compatibility; requires_allocator is a workaround for deduction
// guides in gcc>=11
template <class U, class Metadata, class A, class = detail::requires_allocator<A>>
variable(std::initializer_list<U> list, Metadata&& meta, A alloc)
: variable(list.begin(), list.end(), std::forward<Metadata>(meta), {},
std::move(alloc)) {}
/// Constructor used by algorithm::reduce to shrink and rebin (not for users).
variable(const variable& src, index_type begin, index_type end, unsigned merge)
: metadata_base(src)
, vec_([&] {
assert((end - begin) % merge == 0);
if (options_type::test(option::circular) && !(begin == 0 && end == src.size()))
BOOST_THROW_EXCEPTION(std::invalid_argument("cannot shrink circular axis"));
vector_type vec(src.get_allocator());
vec.reserve((end - begin) / merge);
const auto beg = src.vec_.begin();
for (index_type i = begin; i <= end; i += merge) vec.emplace_back(*(beg + i));
return vec;
}())
, eytzinger_layout_and_eytzinger_binary_search_(vec_) {}
/// Return index for value argument.
index_type index(value_type x) const noexcept {
if (options_type::test(option::circular)) {
const auto a = vec_[0];
const auto b = vec_[size()];
x -= std::floor((x - a) / (b - a)) * (b - a);
}
// upper edge of last bin is inclusive if overflow bin is not present
if (!options_type::test(option::overflow) && x == vec_.back()) return size() - 1;
return eytzinger_layout_and_eytzinger_binary_search_.index(x);
}
std::pair<index_type, index_type> update(value_type x) noexcept {
const auto i = index(x);
if (std::isfinite(x)) {
if (0 <= i) {
if (i < size()) return std::make_pair(i, 0);
const auto d = value(size()) - value(size() - 0.5);
x = std::nextafter(x, (std::numeric_limits<value_type>::max)());
x = (std::max)(x, vec_.back() + d);
vec_.push_back(x);
eytzinger_layout_and_eytzinger_binary_search_.assign(vec_);
return {i, -1};
}
const auto d = value(0.5) - value(0);
x = (std::min)(x, value(0) - d);
vec_.insert(vec_.begin(), x);
eytzinger_layout_and_eytzinger_binary_search_.assign(vec_);
return {0, -i};
}
return {x < 0 ? -1 : size(), 0};
}
/// Return value for fractional index argument.
value_type value(real_index_type i) const noexcept {
if (options_type::test(option::circular)) {
auto shift = std::floor(i / size());
i -= shift * size();
double z;
const auto k = static_cast<index_type>(std::modf(i, &z));
const auto a = vec_[0];
const auto b = vec_[size()];
return (1.0 - z) * vec_[k] + z * vec_[k + 1] + shift * (b - a);
}
if (i < 0) return detail::lowest<value_type>();
if (i == size()) return vec_.back();
if (i > size()) return detail::highest<value_type>();
const auto k = static_cast<index_type>(i); // precond: i >= 0
const real_index_type z = i - k;
// check z == 0 needed to avoid returning nan when vec_[k + 1] is infinity
return (1.0 - z) * vec_[k] + (z == 0 ? 0 : z * vec_[k + 1]);
}
/// Return bin for index argument.
auto bin(index_type idx) const noexcept { return interval_view<variable>(*this, idx); }
/// Returns the number of bins, without over- or underflow.
index_type size() const noexcept { return static_cast<index_type>(vec_.size()) - 1; }
/// Returns the options.
static constexpr unsigned options() noexcept { return options_type::value; }
template <class V, class M, class O, class A>
bool operator==(const variable<V, M, O, A>& o) const noexcept {
const auto& a = vec_;
const auto& b = o.vec_;
return std::equal(a.begin(), a.end(), b.begin(), b.end()) &&
detail::relaxed_equal{}(this->metadata(), o.metadata());
}
template <class V, class M, class O, class A>
bool operator!=(const variable<V, M, O, A>& o) const noexcept {
return !operator==(o);
}
/// Return allocator instance.
auto get_allocator() const { return vec_.get_allocator(); }
template <class Archive>
void serialize(Archive& ar, unsigned /* version */) {
ar& make_nvp("seq", vec_);
ar& make_nvp("meta", this->metadata());
}
private:
vector_type vec_;
boost::histogram::detail::eytzinger_layout_and_eytzinger_binary_search_t<Value>
eytzinger_layout_and_eytzinger_binary_search_;
template <class V, class M, class O, class A>
friend class variable;
};
#if __cpp_deduction_guides >= 201606
template <class T>
variable(std::initializer_list<T>)
-> variable<detail::convert_integer<T, double>, null_type>;
template <class T, class M>
variable(std::initializer_list<T>, M)
-> variable<detail::convert_integer<T, double>,
detail::replace_type<std::decay_t<M>, const char*, std::string>>;
template <class T, class M, unsigned B>
variable(std::initializer_list<T>, M, const option::bitset<B>&)
-> variable<detail::convert_integer<T, double>,
detail::replace_type<std::decay_t<M>, const char*, std::string>,
option::bitset<B>>;
template <class Iterable, class = detail::requires_iterable<Iterable>>
variable(Iterable) -> variable<
detail::convert_integer<
std::decay_t<decltype(*std::begin(std::declval<Iterable&>()))>, double>,
null_type>;
template <class Iterable, class M>
variable(Iterable, M) -> variable<
detail::convert_integer<
std::decay_t<decltype(*std::begin(std::declval<Iterable&>()))>, double>,
detail::replace_type<std::decay_t<M>, const char*, std::string>>;
template <class Iterable, class M, unsigned B>
variable(Iterable, M, const option::bitset<B>&) -> variable<
detail::convert_integer<
std::decay_t<decltype(*std::begin(std::declval<Iterable&>()))>, double>,
detail::replace_type<std::decay_t<M>, const char*, std::string>, option::bitset<B>>;
#endif
} // namespace axis
} // namespace histogram
} // namespace boost
#endif |
The rollback_move_on_exception guard is cool, but maybe we fix it without using that. |
I gave this more thought and I don't want to fix this. I don't like that we have to template the Metadata parameter for this to work. There is a compilation cost associated to all the template instantiations and we try to keep that minimal, too. Instead, the caveat of the design will be documented in the docstring of the ctor. |
We don't have to worry about moving from the iterators, because the iterators will only ever by simple arithmetic types, were moves do not give you an advantage. |
@jhcarl0814 What do you think about this design, which is based on your rollback_move_on_exception idea. It delays the move until the constructor has completed and only performs it if there was no exception. I have to use a helper type in the public API The change of initialization order is a not a problem, since initialization order is an implementation detail. We turn an move/copy-ctor into default ctor + move/copy-assign. That's ok, the MetaData type must be assignable and default constructible. The ultimate conclusion of Hyrum's law is that you cannot change anything ever. Boost does not follow that, because we want to keep some room for improvements. We are not the C++ stdlib. That being said, any change to the API has to be done with care and must not break existing code (without a deprecation period and prior warning). #include <iostream>
#include <cassert>
#include <stdexcept>
#include <boost/core/lightweight_test.hpp>
struct meta {
void msg(const char* msg) {
std::cerr << msg << std::endl;
}
meta() { msg("Default ctor"); }
meta(int v) : value_{v} { msg("Init ctor"); }
meta(const meta& o) : value_{o.value_} { msg("Copy ctor"); }
meta( meta&& o) noexcept {
msg("Move ctor");
value_ = o.value_;
o.value_ = 0;
}
meta& operator=(meta&& o) noexcept {
msg("Move assign");
value_ = o.value_;
o.value_ = 0;
return *this;
}
meta& operator=(const meta& o) {
msg("Copy assign");
value_ = o.value_;
return *this;
}
~meta() {
if (value_)
msg("Destroy");
}
int value_ = 0;
};
template <class T>
struct delayed_forward {
delayed_forward() = default;
delayed_forward(T&& t) : src_{&t} {}
delayed_forward(const T& t) : src_{const_cast<T*>(&t)}, move_{false} {}
void operator()(T& dst) {
if (move_)
dst = std::move(*src_);
else
dst = *src_;
}
T* src_ = nullptr;
bool move_ = true;
};
template <class Meta>
struct axis {
using metadata_type = Meta;
axis(unsigned n, delayed_forward<metadata_type> meta = {}) {
if (n == 0)
throw std::invalid_argument("bad argument");
meta(meta_); // must come after potential exceptions
}
private:
metadata_type meta_;
};
// deduction guide
template <class M>
axis(unsigned, M) -> axis<M>;
int main() {
{ // move with throw
meta m = 1;
try {
axis(0, std::move(m));
} catch(...) {
BOOST_TEST_EQ(m.value_, 1);
}
}
{ // move without throw
meta m = 1;
axis(1, std::move(m));
BOOST_TEST_EQ(m.value_, 0);
}
{ // copy with throw
meta m = 1;
try {
axis(0, m);
} catch(...) {
BOOST_TEST_EQ(m.value_, 1);
}
}
{ // copy without throw
meta m = 1;
axis(1, m);
BOOST_TEST_EQ(m.value_, 1);
}
// deduction guide
axis(1, meta{1});
return boost::report_errors();
} |
@HDembinski "moving from the iterators" is okay, but "moving from the things the iterators point to" is-a side-effect:
The |
Yes that's good. It would be great if we could enforce at compile-time that the developer calls |
I am going to reopen this issue, since it would be good to address this as best as possible. The perfect is the enemy of the good. |
@HDembinski Boost.Multiprecision doc says that One way to force constructor author invoke the move logic only once is to let constructor author pass the function body to #include <iostream>
#include <cassert>
#include <stdexcept>
#include <boost/core/lightweight_test.hpp>
// #include<boost/core/uncaught_exceptions.hpp>
struct meta {
void msg(const char* msg) {
std::cerr << msg << std::endl;
}
meta() { msg("Default ctor"); }
meta(int v) : value_{v} { msg("Init ctor"); }
meta(const meta& o) : value_{o.value_} { msg("Copy ctor"); }
meta( meta&& o) noexcept {
msg("Move ctor");
value_ = o.value_;
o.value_ = 0;
}
meta& operator=(meta&& o) noexcept {
msg("Move assign");
value_ = o.value_;
o.value_ = 0;
return *this;
}
meta& operator=(const meta& o) {
msg("Copy assign");
value_ = o.value_;
return *this;
}
~meta() {
if (value_)
msg("Destroy");
}
int value_ = 0;
};
template<typename T>
T forward_decltype_auto(std::remove_reference_t<T> &arg) //https://stackoverflow.com/a/57440814/8343353
{
return std::forward<T>(arg);
}
template <class T>
struct delayed_forward {
delayed_forward() = default;
delayed_forward(T&& t) : src_{&t} {}
delayed_forward(const T& t) : src_{const_cast<T*>(&t)}, move_{false} {}
template<typename F>
decltype(auto) function_body(T& dst, F&&f)
{
// struct guard_t
// {
// delayed_forward* this_;
// T& dst_;
// unsigned int count_ = boost::core::uncaught_exceptions();
// ~guard_t()
// {
// if(count_ == boost::core::uncaught_exceptions())
// {
// if(this_->src_ != nullptr)
// {
// if (this_->move_)
// dst_ = std::move(*this_->src_);
// else
// dst_ = *this_->src_;
// }
// }
// }
// }guard{this,dst};
// return std::forward<F>(f);
try
{
decltype(auto)result = std::forward<F>(f);
if(src_ != nullptr)
{
if (move_)
dst = std::move(*src_);
else
dst = *src_;
}
return forward_decltype_auto<decltype(result)>(result);
}
catch(...)
{
throw;
}
}
T* src_ = nullptr;
bool move_ = true;
};
template <class Meta = meta>
struct axis {
using metadata_type = Meta;
axis(unsigned n, delayed_forward<metadata_type> meta = {}) {
meta.function_body(meta_,[&]
{
if (n == 0)
throw std::invalid_argument("bad argument");
});
}
private:
metadata_type meta_;
};
// deduction guide
template <class M>
axis(unsigned, M) -> axis<M>;
int main() {
{
try {
axis(0);
} catch(...) {
}
}
std::cerr<<'\n';
{
axis(1);
}
std::cerr<<'\n';
{ // move with throw
meta m = 1;
try {
axis(0, std::move(m));
} catch(...) {
BOOST_TEST_EQ(m.value_, 1);
}
}
std::cerr<<'\n';
{ // move without throw
meta m = 1;
axis(1, std::move(m));
BOOST_TEST_EQ(m.value_, 0);
}
std::cerr<<'\n';
{ // copy with throw
meta m = 1;
try {
axis(0, m);
} catch(...) {
BOOST_TEST_EQ(m.value_, 1);
}
}
std::cerr<<'\n';
{ // copy without throw
meta m = 1;
axis(1, m);
BOOST_TEST_EQ(m.value_, 1);
}
std::cerr<<'\n';
// deduction guide
axis(1, meta{1});
return boost::report_errors();
} |
@jhcarl0814 Just to note, I am currently discussing this topic on the boost-dev mailing list. |
template <class It, class = detail::requires_iterator<It>> variable(It begin, It end, metadata_type meta = {}, options_type options = {}, allocator_type alloc = {})
does not provide strong exception guarantee: if user writesvariable(it_begin, it_end, std::move(str))
and constructor body throws then the value insidestr
is lost (the value was moved into parameter and then data member before the exception). So one can not writewhile(std::cin>>str>>...){ try{ variable(..., ..., str); ... }catch(...){} }
and has to take on the burden of saving thestr
elsewhere (so the user do not have to type it again) when other constructor arguments might be illegal and have to be typed again.Originally posted by @jhcarl0814 in #372 (comment)
The text was updated successfully, but these errors were encountered: