[PATCH] D50710: [ADT] Fix Optional ABI mismatch between clang & gcc

[Kenneth Benzie via Phabricator via llvm-commits]

kbenzie added inline comments.


================
Comment at: include/llvm/ADT/Optional.h:120
   OptionalStorage &operator=(const T &y) {
-    *reinterpret_cast<T *>(storage.buffer) = y;
-    hasVal = true;
+    if (hasVal) {
+      // Use std::addressof to silence strict-aliasing warnings from gcc.
----------------
timshen wrote:
> timshen wrote:
> > Will this suffice?
> > `if (hasVal) { memcpy(&storage.buffer, &y.storage.buffer, sizeof(T)); }`
> Sorry, I meant
>     hasVal = y.hasVal;
>     if (hasVal) { memcpy(...); }
This assignment operator does not take a `const OptionalStorage&` so `y.hasVal` is not a valid expression since `T` is the type of the object to be copied into  `storage.buffer` e.g. for `OptionalStorage<int>` 
then `T` is `int`.

A [[ https://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20180122/519089.html | previous attempt ]] to fix the GCC miscompiles used `memmove` which was only reported to fix GCC 5.4 release builds.

The expression `*reinterpret_cast<T *>(storage.buffer) = y` is undefined behaviour in the case where a valid object is not held in `storage.buffer` according to C++ rules which lead to this implementation.


Repository:
  rL LLVM

https://reviews.llvm.org/D50710