[libc-commits] [PATCH] D129920: [libc] Trivial implementation of std::optional

[Guillaume Chatelet via Phabricator via libc-commits]

gchatelet added inline comments.


================
Comment at: libc/src/__support/CPP/Optional.h:44
+
+  Optional(const Optional &t) : StoredValue(t.value()), InUse(true) {}
+
----------------
jeffbailey wrote:
> zero9178 wrote:
> > This copy constructor causes undefined behaviour in the case that `t` is not does not have a value. Shouldn't this be conditionally copying `t.value()` into `StoredValue` depending on whether `t.has_value()`?
> > 
> > (I am an outsider to the libc part of LLVM, but I am guessing you'll probably also want a copy assignment operator acting similarily)
> Oh bugger.  This probably explains why everyone does the storage as a sub object.
> 
> Happily, I also found an implementation in llvm/include/llvm/ADT/optional.h that's a bit simpler than the one in libcxx, so I'll go with that.  Gimme another day.
Yes absolutely. And because of the union you'll have to initialize either `Placeholder` or `StoredValue`, so I think the code has to move into the body of the copy constructor.

And yes, if you have the copy ctor, you also want the copy assignment operator.

Thx for noticing @zero9178 .


================
Comment at: libc/src/__support/CPP/Optional.h:21-24
+// This is very simple implementation of the std::optional class.  There are a
+// number of guarantees in the standard that are not made here.
+//
+// This class will be extended as needed in future.
----------------
jeffbailey wrote:
> gchatelet wrote:
> > sivachandra wrote:
> > > gchatelet wrote:
> > > > I would list the requirements for `T` here, AFAICT:
> > > >  - default constructible
> > > >  - copy constructible
> > > >  - copy assignable
> > > >  - destructible
> > > I think Guilllaume is just asking you to list them for now. IIUC, except for the default constructible requirement, they are already enforced?
> > Yes exactly it's just to document the expectations and indeed "default constructible" might not  be required (because of the `union`)
> > Ideally this would be checked upfront in the class body with some `static_assert`s and CPP traits. The aim here is to make the failing requirements obvious.
> I'm not sure where to put static_asserts in here.  I think if it's not copy constructable/assignable or not destructable, compilation will simply fail right now.  I couldn't think of what static_asserts to put in beyond this.
> 
> Feedback is always welcome, thanks!
I would have put the `static_assert` in the body of the class so it fails right before it's used in any method.
That said I'm not sure we have the type traits handy (since we have to reimplement most of them).
I don't want to block the patch any longer here. We can do it as a follow up.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129920/new/

https://reviews.llvm.org/D129920