[libc-commits] [PATCH] D150211: [libc] Rework of cpp::optional to support more types

[Mikhail Ramalho via Phabricator via libc-commits]

mikhail.ramalho added a comment.
Herald added a subscriber: wangpc.

In D150211#4396897 <https://reviews.llvm.org/D150211#4396897>, @michaelrj wrote:

> Ideally we would not like to replace the current `optional` implementation. Instead we would prefer to modify the current `optional` so that it supports the features that we need, and only replace it if modification isn't practical. Can you explain why you think that modifying the existing implementation of `optional` cannot work for your use case?

Hey Michael, sorry for taking so long to answer here.

So, although this PR includes several line changes, the overall implementation is not that different from the current optional in libc.

The major differences are:

1. In the storage class:
2. A new base class called `OptionalStorageBase<T>` for `OptionalStorage<T>`. Mainly to avoid duplicated code in derived classes.
3. Specialization of `OptionalStorage<T>` for when `T` is not trivially destructive, not trivially copy assignable/copy constructible, and not trivially move assignable/move constructible, + some combinations of these properties.

2. In the optional class:
3. A new base class called `OptionalBaseImpl<T>` for `OptionalBase<T>` and `optional<T>`, also mainly to avoid duplicated code in derived classes.
4. A new class `OptionalBaseImpl<T>` and specialization for when `T` is not copy constructible, and not move constructible,  + some combinations of these properties.
5. Changed the `optional<T>` class to inherit from `OptionalBase<T>` and support objects with the properties above.

May I ask you to reconsider this PR as is?

I _think_ I can reduce it to support only the case we need (`UInt<T>` is not trivially destructible), but when I ported this code from llvm::optional, I decided to support all cases in case we need it for the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150211