Re: [Patch] Implementation of n3793 <experimental/optional>

[Jonathan Wakely <jwakely dot gcc at gmail dot com>]

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?

Attachment: tweaks.txt
Description: Text document