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

[Jeff Bailey via Phabricator via libc-commits]

jeffbailey marked an inline comment as done.
jeffbailey added inline comments.


================
Comment at: libc/src/__support/CPP/Optional.h:44
+
+  Optional(const Optional &t) : StoredValue(t.value()), InUse(true) {}
+
----------------
gchatelet wrote:
> 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 .
I think this is done now and covered with a test.


================
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.
----------------
gchatelet wrote:
> 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.
I'd like to do this as a follow-up.  I wrestled with type_traits here for a bunch of the last week and we don't have the ones that we need and I don't understand them well enough to implement them.  (And I'd like to unblock two patches that want to use this)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129920