Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

Make llvm::Optional<T> trivially copyable when T is trivially copyable

Authored by serge-sans-paille on Jan 23 2019, 6:12 AM.



This is an ever-recurring issue (see and but I believe that thanks to we can now ship a decent implementation of this.

Basically the fact that llvm::is_trivially_copyable has a consistent behavior across compilers should prevent any ABI issue, and using in-place new instead of memcpy should keep compiler bugs away.

Diff Detail


Event Timeline

tstellar added a subscriber: tstellar.

This validates with gcc 8.2. Given the history of llvm::Optional, I'm going to try to build with other compilers too.

Also validates ok with clang 6.0.1 and gcc 4.8.5

Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 5:06 AM

I think you may be able to do this in a slightly nicer way.

If you move the definitions of the relevant special member functions to be out-of-line, I think you can remove the class-level specialization entirely, and simply explicitly specialize (out-of-line) the definitions of the relevant special member functions, with the trivial case specializing them to = default definitions, and the non-trivial case specializing to their current definitions. Make sense?

My only concern is that I think there were some dark corner cases around defined-as-defaulted but not declared-as-defaulted. I don't know if they impact triviality in ways that cause problems.

@chandlerc there's indeed some redundancy that can be removed unfortunately, as you feared, defaulting seems incompatible with per-member specialization, or at least it hits my c++ knowledge limit.

Here is my attempt:
I have an alternate version using inheritance under testing, I'll upload the patch during the day.

@chandlerc patch updated, using inheritance to avoid too much duplication.

chandlerc accepted this revision.Feb 12 2019, 11:54 PM


Thanks for seeing through this .... very non-trivial effort! =D Also, as I'm sure you know by now, will need to watch the bots carefully. Sadly, some may fall over.

This revision is now accepted and ready to land.Feb 12 2019, 11:54 PM
This revision was automatically updated to reflect the committed changes.

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:

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 that uses union as storage and provides cleaner encapsulation between
Optional and OptionalStorage, followed by 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.