This is an archive of the discontinued LLVM Phabricator instance.

[libc] Trivial implementation of std::optional
ClosedPublic

Authored by jeffbailey on Jul 15 2022, 10:38 PM.

Details

Summary

This class has only the minimum functionality in it to provide what the
TZ variable parsing needs. In particular, the standard makes guarantees
about how trivial the destructors are, throws an expception if it's used
incorrectly, etc. There are also missing features.

Tested:
Trivial testsuite added, and use in development.

Diff Detail

Event Timeline

jeffbailey created this revision.Jul 15 2022, 10:38 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 15 2022, 10:38 PM
jeffbailey requested review of this revision.Jul 15 2022, 10:38 PM
jeffbailey added a subscriber: rtenneti.

Hi! I'm not expecting that this is in final shape, but I wanted to ask for an initial set of feedback from you. The test suite certainly needs expanding, but I'd love to get your initial thoughts on this approach, etc.

Thanks!

tschuett added a comment.EditedJul 16 2022, 12:58 AM

Do you need the union and the Placeholder? In reset you reset the StoredValue. This implies something about T.

I would add asserts to catch misbehaviour.

The LLVM optional uses a union for storage.

libc/src/__support/CPP/Optional.h
26

The private is redundant or for documentation.

Do you need the union and the Placeholder? In reset you reset the StoredValue. This implies something about T.

I think I need the union because I don't want to construct the object if I'm not using it as in this case:

std::optional<std::string> foo() {
  return std::nullopt;
}

Does a union still have that property if it only has a single member? Or is there a different approach?

I would add asserts to catch misbehaviour.

Will do. It's almost tempting to also implement std::variant and use that since we'll probably want it later anyway.

The LLVM optional uses a union for storage.

Right. They're using the same approach that I am with:

union
{
    char __null_state_;
    value_type __val_;
};

I've just used the words Placeholder and StoredValue. They're using __engaged where I'm using InUse.

If the LLVM optional uses a union, then it is probably a good idea.

I guess you are more interested in trivially copyable types.

If the LLVM optional uses a union, then it is probably a good idea.

I guess you are more interested in trivially copyable types.

It solves our current use, anyway. =) I could add the && versions if folks think they'll get used soon (or that this will be confusing to not have them for now). For this I was focused on what's needed for our case of implementing the TZ variable parsing (we use this for int, struct, and a string_view).

Thanks!

You'd also need to adjust the Bazel config.

libc/src/__support/CPP/Optional.h
15

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?

21–24

I would list the requirements for T here, AFAICT:

  • default constructible
  • copy constructible
  • copy assignable
  • destructible
47

You can drop noexpect here and below, exceptions are disabled throughout llvm-libc

Updating with some review comments addressed, but not the asserts or the
testsuite, etc. Posting this in case you want this committed to unblock
https://reviews.llvm.org/D129918 and the fix-ups to come in a subsequent
change.

On review, saw two other review comments that I could address easily.

jeffbailey marked 2 inline comments as done.Jul 19 2022, 10:36 PM

You'd also need to adjust the Bazel config.

Done.

Updating with some review comments addressed, but not the asserts or the
testsuite, etc.

I think, starting with a simple implementation like this is OK. IMO, we do not have to mimic std::optional - IIUC, we want something similar for our convenience but not exactly equal. So, this is LGTM after expanding the testsuite. Please wait for Guillaume though.

libc/src/__support/CPP/Optional.h
21–24

I think Guilllaume is just asking you to list them for now. IIUC, except for the default constructible requirement, they are already enforced?

gchatelet accepted this revision.Jul 20 2022, 12:49 AM

Please add the requirements on T for Optional as documentation and then this is good to go.

libc/src/__support/CPP/Optional.h
21–24

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_asserts and CPP traits. The aim here is to make the failing requirements obvious.

This revision is now accepted and ready to land.Jul 20 2022, 12:49 AM

It's late enough that I'm going to wait until morning to commit this, in case
Guillaume or others want to take a look at the test suite and comments.

Otherwise I already have an LGTM and will commit in the morning.

jeffbailey marked 2 inline comments as done.Jul 20 2022, 11:45 PM
jeffbailey added inline comments.
libc/src/__support/CPP/Optional.h
21–24

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!

zero9178 added inline comments.
libc/src/__support/CPP/Optional.h
45

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)

jeffbailey added inline comments.Jul 21 2022, 7:51 AM
libc/src/__support/CPP/Optional.h
45

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.

gchatelet added inline comments.Jul 21 2022, 9:14 AM
libc/src/__support/CPP/Optional.h
21–24

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.

45

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 .

lntue added a subscriber: lntue.Jul 22 2022, 7:38 PM

Added two tests to test code review comment that noticed that an unset
optional would become set on assignment. Then fixed the code for
that situation by factoring our storage into a consistent object.

Since the decision to use the C++ casing, changed Optional,
Nullopt and NulloptT to optional, nullopt, and nullopt_t for this
upload.

Oops, I noticed that I didn't have clang-format installed. This
diff fixes one formatting error.

jeffbailey marked an inline comment as done.Jul 30 2022, 12:55 PM
jeffbailey added inline comments.
libc/src/__support/CPP/Optional.h
21–24

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)

45

I think this is done now and covered with a test.

Unknown Object (User) added a subscriber: Unknown Object (User).Jul 30 2022, 1:32 PM

The Kingdom Valley is a community of people who want to make a difference in their lives and in the world. They believe that through living an authentic life and creating a strong family and a strong community, they can achieve a better future for themselves and for those around them. The community has been growing steadily since its inception in 1998. It now has over 2000 members, with new members joining every day.

jeffbailey marked 5 inline comments as done.Aug 3 2022, 10:25 PM

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

libc/src/__support/CPP/Optional.h
15

Per the separate discussion, keeping as nullopt_t

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

21–24

Doing as a follow-up per this comment.

45

This code rewritten.

This revision was landed with ongoing or failed builds.Aug 4 2022, 7:52 PM
This revision was automatically updated to reflect the committed changes.
jeffbailey marked an inline comment as done.