Page MenuHomePhabricator

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

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

Details

Summary

This is an ever-recurring issue (see https://bugs.llvm.org/show_bug.cgi?id=39427 and https://bugs.llvm.org/show_bug.cgi?id=35978) but I believe that thanks to https://reviews.llvm.org/D54472 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

Repository
rL LLVM

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: https://godbolt.org/z/9_Lykd
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

LGTM

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 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.