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

[Jeff Bailey via Phabricator via libc-commits]

jeffbailey marked 5 inline comments as done.
jeffbailey added a comment.

I think this closes out the comments.  I'll leave this another day before submitting.  Thanks all!



================
Comment at: libc/src/__support/CPP/Optional.h:44
+
+  Optional(const Optional &t) : StoredValue(t.value()), InUse(true) {}
+
----------------
jeffbailey wrote:
> 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.
This code rewritten.


================
Comment at: libc/src/__support/CPP/Optional.h:15
+
+// Trivial Nullopt_t struct.
+struct Nullopt_t {};
----------------
gchatelet wrote:
> According to our current style guide that would be `NulloptT`.
> Note that I would be in favor of using the STL names whenever we're re-implementing them but that would be a separate discussion.
> 
> Do you want to limit it to POD types? Or to primitive types?
Per the separate discussion, keeping as nullopt_t

I'll add more constraints to this once we have more type traits in place.


================
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:
> > 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)
Doing as a follow-up per this comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129920