[PATCH] D57097: Make llvm::Optional<T> trivially copyable when T is trivially copyable

[serge via Phabricator via llvm-commits]

serge-sans-paille added a comment.

Some follow-up on this review: I had to revert the associated commit because it was breaking on buildbot, on test case I failed to reproduce locally.

My educated guess on that subject is that original llvm::Optional implementation had some issues, namely:

- improper encapsulation (internal buffer exposed to llvm::Optional)
- UB as far as I understand this post https://stackoverflow.com/questions/27003727/does-this-really-break-strict-aliasing-rules
- some typo (std::forward used where std::move should be used)

This made implementing the property « optional<T> is trivially copyable  if T is trivially copyable a very difficult task.
I experimented a bit and ended up commiting https://reviews.llvm.org/rL354246 that uses union as storage and provides cleaner encapsulation between
Optional and OptionalStorage, followed by https://reviews.llvm.org/rL354246 that implements the same idea as current review.

I'm pretty sure there's room for improvements, at least I hope we're on firmer ground now.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57097