[Patch] Implementation of n3793 <experimental/optional>
Jonathan Wakely
jwakely.gcc@gmail.com
Thu Oct 31 14:18:00 GMT 2013
On 18 October 2013 07:06, Luc Danton wrote:
> Hello everyone,
>
> I have completed the copyright assignment process. I originally submitted a
> candidate patch to implement <optional> at the end of July
> (http://gcc.gnu.org/ml/libstdc++/2013-07/msg00137.html). This was against
> n3672,
> i.e. revision 4 of the optional proposal. As it so happens, n3793 was
> published
> Thursday as a 5th revision.
I've finally finished going through your patches - it's excellent
work, thanks very much.
> Important things that aren't quite correct and needs reviewing/fixing:
> - include/Makefile.am, which I've modified due to putting the header in
> include/experimental/optional -- tests do compile and pass but I'm afraid
> I've otherwise botched it
It looks OK, except that the new header isn't installed with the library :-)
That's easily fixed.
> - include/Makefile.in needs to be regenerated
I can do that when committing it.
> Not as important:
> - changes to testsuite/libstdc++-dg/conformance.exp could use a review,
> because
> I've put the tests in testsuite/experimental/optional/
That seems OK to me. We can always move things around under
experimental later if we want to separate things according to which TS
they appear in.
> - the ABI-related matters of bad_optional_access and
> __throw_bad_optional_access will need to be settled somehow (introduce
> src/experimental/optional.cc and
> src/experimental/compatibility_functexcept.cc? I can't really say), and
> until
> then I have left the associated function definitions inline in the header
> --
> they are marked by the only two XXX items of the file
That's OK for now, but let's remember to revisit it some time!
We usually don't bother marking definitions "inline" if they are
implicitly inline due to being defined in the class body.
+ inline void
+ __throw_bad_optional_access(const char*)
+ __attribute__((__noreturn__));
+
+ // XXX Does not belong here.
+ inline void
+ __throw_bad_optional_access(const char* __s)
+ { _GLIBCXX_THROW_OR_ABORT(bad_optional_access(__s)); }
I think we should also remove the "inline" specifier on the first
declaration here, because when we move the definition to a .cc file it
won't be inline anymore.
The comment on _Has_addressof has a stray asterisk:
+ * declval<_Tp * const&>().operator&()).
I changed s/responsability/responsibility/ in a couple of places.
Could the SFINAE constraints on optional<T>::operator= and
optional<T>::emplace be put on the return type, instead of as a
parameter pack of the function? Making it a parameter pack allows
users to misuse the function by providing explicit template argument
lists e.g. opt.emplace<int, 1, 2, 3, 4>(1) will compile, disabling the
SFINAE constraint. Arguably this falls into the category of "you
can't protect against sufficiently motivated idiots" but putting the
constraint on the return type makes it a non-issue.
Shouldn't optional::swap() be using _M_construct(std::move(...)) when
only one of the objects is engaged?
I've attached a patch with these changes (except my suggestion for the
constraints on operator= and emplace), and some re-indenting. These
are all trivial and so I think this is ready to commit. I can go ahead
and do that for you if you like?
-------------- next part --------------
diff --git a/libstdc++-v3/include/Makefile.am b/libstdc++-v3/include/Makefile.am
index e107f31..505679a 100644
--- a/libstdc++-v3/include/Makefile.am
+++ b/libstdc++-v3/include/Makefile.am
@@ -1289,6 +1289,9 @@ install-headers:
$(mkinstalldirs) $(DESTDIR)${gxx_include_dir}/${decimal_builddir}
for file in ${decimal_headers}; do \
$(INSTALL_DATA) $${file} $(DESTDIR)${gxx_include_dir}/${decimal_builddir}; done
+ $(mkinstalldirs) $(DESTDIR)${gxx_include_dir}/${experimental_builddir}
+ for file in ${experimental_headers}; do \
+ $(INSTALL_DATA) $${file} $(DESTDIR)${gxx_include_dir}/${experimental_builddir}; done
$(mkinstalldirs) $(DESTDIR)${gxx_include_dir}/${c_base_builddir}
for file in ${c_base_headers}; do \
$(INSTALL_DATA) $${file} $(DESTDIR)${gxx_include_dir}/${c_base_builddir}; done
@@ -1334,9 +1337,10 @@ clean-local:
# directory. (This is more of an example of how this kind of rule can
# be made.)
.PRECIOUS: $(std_headers) $(c_base_headers) $(tr1_headers) $(tr2_headers)
- $(decimal_headers) $(ext_headers)
+ $(decimal_headers) $(ext_headers) $(experimental_headers)
$(std_headers): ; @:
$(c_base_headers): ; @:
$(tr1_headers): ; @:
$(decimal_headers): ; @:
$(ext_headers): ; @:
+$(experimental_headers): ; @:
diff --git a/libstdc++-v3/include/experimental/optional b/libstdc++-v3/include/experimental/optional
index 2811b01..d49e10a 100644
--- a/libstdc++-v3/include/experimental/optional
+++ b/libstdc++-v3/include/experimental/optional
@@ -107,16 +107,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
{
public:
// XXX Should not be inline
- inline explicit bad_optional_access(const string& __arg)
- : logic_error(__arg) { }
+ explicit bad_optional_access(const string& __arg) : logic_error(__arg) { }
- inline explicit bad_optional_access(const char* __arg)
- : logic_error(__arg) { }
+ explicit bad_optional_access(const char* __arg) : logic_error(__arg) { }
- inline virtual ~bad_optional_access() noexcept = default;
+ virtual ~bad_optional_access() noexcept = default;
};
- inline void
+ void
__throw_bad_optional_access(const char*)
__attribute__((__noreturn__));
@@ -164,7 +162,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
/**
* @brief Class template that holds the necessary state for @ref optional
- * and that has the responsability for construction and the special members.
+ * and that has the responsibility for construction and the special members.
*
* Such a separate base class template is necessary in order to
* conditionally enable the special members (e.g. copy/move constructors).
@@ -175,7 +173,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
* @see optional, _Enable_special_members
*/
template<typename _Tp, bool _ShouldProvideDestructor =
- !is_trivially_destructible<_Tp>::value>
+ !is_trivially_destructible<_Tp>::value>
class _Optional_base
{
private:
@@ -189,10 +187,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
// Constructors for disengaged optionals.
constexpr _Optional_base() noexcept
- : _M_empty {} { }
+ : _M_empty{} { }
constexpr _Optional_base(nullopt_t) noexcept
- : _Optional_base {} { }
+ : _Optional_base{} { }
// Constructors for engaged optionals.
constexpr _Optional_base(const _Tp& __t)
@@ -333,10 +331,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
public:
constexpr _Optional_base() noexcept
- : _M_empty {} { }
+ : _M_empty{} { }
constexpr _Optional_base(nullopt_t) noexcept
- : _Optional_base {} { }
+ : _Optional_base{} { }
constexpr _Optional_base(const _Tp& __t)
: _M_payload(__t), _M_engaged(true) { }
@@ -487,7 +485,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
public:
using value_type = _Tp;
- // _Optional_base has the responsability for construction.
+ // _Optional_base has the responsibility for construction.
using _Base::_Base;
// [X.Y.4.3] (partly) Assignment.
@@ -519,15 +517,15 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
}
template<typename... _Args>
- void
- emplace(_Args&&... __args)
- {
- static_assert(is_constructible<_Tp, _Args&&...>(),
- "Cannot emplace value type from arguments.");
+ void
+ emplace(_Args&&... __args)
+ {
+ static_assert(is_constructible<_Tp, _Args&&...>(),
+ "Cannot emplace value type from arguments.");
- this->_M_reset();
- this->_M_construct(std::forward<_Args>(__args)...);
- }
+ this->_M_reset();
+ this->_M_construct(std::forward<_Args>(__args)...);
+ }
template<typename _Up,
typename... _Args,
@@ -535,12 +533,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
initializer_list<_Up>&,
_Args&&...>::value,
int>::type...>
- void
- emplace(initializer_list<_Up> __il, _Args&&... __args)
- {
- this->_M_reset();
- this->_M_construct(__il, std::forward<_Args>(__args)...);
- }
+ void
+ emplace(initializer_list<_Up> __il, _Args&&... __args)
+ {
+ this->_M_reset();
+ this->_M_construct(__il, std::forward<_Args>(__args)...);
+ }
// [X.Y.4.2] Destructor is implicit, implemented in _Optional_base.
@@ -556,12 +554,12 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
swap(this->_M_get(), __other._M_get());
else if (this->_M_is_engaged())
{
- __other._M_construct(this->_M_get());
+ __other._M_construct(std::move(this->_M_get()));
this->_M_destruct();
}
else if (__other._M_is_engaged())
{
- this->_M_construct(__other._M_get());
+ this->_M_construct(std::move(__other._M_get()));
__other._M_destruct();
}
}
@@ -607,30 +605,30 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
}
template<typename _Up>
- constexpr _Tp
- value_or(_Up&& __u) const&
- {
- static_assert(is_copy_constructible<_Tp>()
- && is_convertible<_Up&&, _Tp>(),
- "Cannot return value.");
-
- return this->_M_is_engaged() ?
- this->_M_get() :
- static_cast<_Tp>(std::forward<_Up>(__u));
- }
+ constexpr _Tp
+ value_or(_Up&& __u) const&
+ {
+ static_assert(is_copy_constructible<_Tp>()
+ && is_convertible<_Up&&, _Tp>(),
+ "Cannot return value.");
+
+ return this->_M_is_engaged() ?
+ this->_M_get() :
+ static_cast<_Tp>(std::forward<_Up>(__u));
+ }
template<typename _Up>
- _Tp
- value_or(_Up&& __u) &&
- {
- static_assert( is_move_constructible<_Tp>()
- && is_convertible<_Up&&, _Tp>(),
- "Cannot return value." );
-
- return this->_M_is_engaged() ?
- std::move(this->_M_get()) :
- static_cast<_Tp>(std::forward<_Up>(__u));
- }
+ _Tp
+ value_or(_Up&& __u) &&
+ {
+ static_assert( is_move_constructible<_Tp>()
+ && is_convertible<_Up&&, _Tp>(),
+ "Cannot return value." );
+
+ return this->_M_is_engaged() ?
+ std::move(this->_M_get()) :
+ static_cast<_Tp>(std::forward<_Up>(__u));
+ }
};
// [X.Y.8] Comparisons between optional values.
More information about the Libstdc++
mailing list