<div dir="ltr">What was the UB/problem with the existing implementation (<span style="color:rgb(33,33,33)">AlignedCharArrayUnion) - and/or is std::aligned_storage an option?</span></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Feb 17, 2019 at 2:40 PM Serge Guelton via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: serge_sans_paille<br>
Date: Sun Feb 17 14:41:14 2019<br>
New Revision: 354238<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=354238&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=354238&view=rev</a><br>
Log:<br>
[NFC] Better encapsulation of llvm::Optional Storage<br>
<br>
Second attempt, trying to navigate out of the UB zone using<br>
union for storage instead of raw bytes.<br>
<br>
I'm prepared to revert that commit as soon as validation breaks,<br>
which is likely to happen.<br>
<br>
<br>
Modified:<br>
llvm/trunk/include/llvm/ADT/Optional.h<br>
<br>
Modified: llvm/trunk/include/llvm/ADT/Optional.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/Optional.h?rev=354238&r1=354237&r2=354238&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/Optional.h?rev=354238&r1=354237&r2=354238&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/ADT/Optional.h (original)<br>
+++ llvm/trunk/include/llvm/ADT/Optional.h Sun Feb 17 14:41:14 2019<br>
@@ -29,83 +29,89 @@ namespace llvm {<br>
class raw_ostream;<br>
<br>
namespace optional_detail {<br>
+<br>
+struct in_place_t {};<br>
+<br>
/// Storage for any type.<br>
-template <typename T, bool = is_trivially_copyable<T>::value> struct OptionalStorage {<br>
- AlignedCharArrayUnion<T> storage;<br>
- bool hasVal = false;<br>
-<br>
- OptionalStorage() = default;<br>
-<br>
- OptionalStorage(const T &y) : hasVal(true) { new (storage.buffer) T(y); }<br>
- OptionalStorage(const OptionalStorage &O) : hasVal(O.hasVal) {<br>
- if (hasVal)<br>
- new (storage.buffer) T(*O.getPointer());<br>
- }<br>
- OptionalStorage(T &&y) : hasVal(true) {<br>
- new (storage.buffer) T(std::forward<T>(y));<br>
- }<br>
- OptionalStorage(OptionalStorage &&O) : hasVal(O.hasVal) {<br>
- if (O.hasVal) {<br>
- new (storage.buffer) T(std::move(*O.getPointer()));<br>
- }<br>
- }<br>
+template <typename T, bool = is_trivially_copyable<T>::value><br>
+class OptionalStorage {<br>
+ union {<br>
+ char empty;<br>
+ T value;<br>
+ };<br>
+ bool hasVal;<br>
<br>
- OptionalStorage &operator=(T &&y) {<br>
- if (hasVal)<br>
- *getPointer() = std::move(y);<br>
- else {<br>
- new (storage.buffer) T(std::move(y));<br>
- hasVal = true;<br>
- }<br>
- return *this;<br>
- }<br>
- OptionalStorage &operator=(OptionalStorage &&O) {<br>
- if (!O.hasVal)<br>
- reset();<br>
- else {<br>
- *this = std::move(*O.getPointer());<br>
- }<br>
- return *this;<br>
- }<br>
+public:<br>
+ ~OptionalStorage() { reset(); }<br>
<br>
- // FIXME: these assignments (& the equivalent const T&/const Optional& ctors)<br>
- // could be made more efficient by passing by value, possibly unifying them<br>
- // with the rvalue versions above - but this could place a different set of<br>
- // requirements (notably: the existence of a default ctor) when implemented<br>
- // in that way. Careful SFINAE to avoid such pitfalls would be required.<br>
- OptionalStorage &operator=(const T &y) {<br>
- if (hasVal)<br>
- *getPointer() = y;<br>
- else {<br>
- new (storage.buffer) T(y);<br>
- hasVal = true;<br>
+ OptionalStorage() noexcept : empty(), hasVal(false) {}<br>
+<br>
+ OptionalStorage(OptionalStorage const &other) : OptionalStorage() {<br>
+ if (other.hasValue()) {<br>
+ emplace(other.value);<br>
}<br>
- return *this;<br>
}<br>
- OptionalStorage &operator=(const OptionalStorage &O) {<br>
- if (!O.hasVal)<br>
- reset();<br>
- else<br>
- *this = *O.getPointer();<br>
- return *this;<br>
+ OptionalStorage(OptionalStorage &&other) : OptionalStorage() {<br>
+ if (other.hasValue()) {<br>
+ emplace(std::move(other.value));<br>
+ }<br>
}<br>
<br>
- ~OptionalStorage() { reset(); }<br>
+ template <class... Args><br>
+ explicit OptionalStorage(in_place_t, Args &&... args)<br>
+ : value(std::forward<Args>(args)...), hasVal(true) {}<br>
<br>
- void reset() {<br>
+ void reset() noexcept {<br>
if (hasVal) {<br>
- (*getPointer()).~T();<br>
+ value.~T();<br>
hasVal = false;<br>
}<br>
}<br>
<br>
- T *getPointer() {<br>
+ bool hasValue() const noexcept { return hasVal; }<br>
+<br>
+ T &getValue() LLVM_LVALUE_FUNCTION noexcept {<br>
assert(hasVal);<br>
- return reinterpret_cast<T *>(storage.buffer);<br>
+ return value;<br>
}<br>
- const T *getPointer() const {<br>
+ T const &getValue() const LLVM_LVALUE_FUNCTION noexcept {<br>
assert(hasVal);<br>
- return reinterpret_cast<const T *>(storage.buffer);<br>
+ return value;<br>
+ }<br>
+#if LLVM_HAS_RVALUE_REFERENCE_THIS<br>
+ T &&getValue() && noexcept {<br>
+ assert(hasVal);<br>
+ return std::move(value);<br>
+ }<br>
+#endif<br>
+<br>
+ template <class... Args> void emplace(Args &&... args) {<br>
+ reset();<br>
+ ::new ((void *)std::addressof(value)) T(std::forward<Args>(args)...);<br>
+ hasVal = true;<br>
+ }<br>
+<br>
+ OptionalStorage &operator=(T const &y) {<br>
+ return (*this) = OptionalStorage(in_place_t{}, y);<br>
+ }<br>
+ OptionalStorage &operator=(T &&y) {<br>
+ return (*this) = OptionalStorage(in_place_t{}, std::move(y));<br>
+ }<br>
+<br>
+ OptionalStorage &operator=(OptionalStorage const &other) {<br>
+ if (other.hasValue())<br>
+ emplace(other.getValue());<br>
+ else<br>
+ reset();<br>
+ return *this;<br>
+ }<br>
+<br>
+ OptionalStorage &operator=(OptionalStorage &&other) {<br>
+ if (other.hasValue())<br>
+ emplace(std::move(other).getValue());<br>
+ else<br>
+ reset();<br>
+ return *this;<br>
}<br>
};<br>
<br>
@@ -120,10 +126,10 @@ public:<br>
constexpr Optional() {}<br>
constexpr Optional(NoneType) {}<br>
<br>
- Optional(const T &y) : Storage(y) {}<br>
+ Optional(const T &y) : Storage(optional_detail::in_place_t{}, y) {}<br>
Optional(const Optional &O) = default;<br>
<br>
- Optional(T &&y) : Storage(std::forward<T>(y)) {}<br>
+ Optional(T &&y) : Storage(optional_detail::in_place_t{}, std::move(y)) {}<br>
Optional(Optional &&O) = default;<br>
<br>
Optional &operator=(T &&y) {<br>
@@ -134,9 +140,7 @@ public:<br>
<br>
/// Create a new object by constructing it in place with the given arguments.<br>
template <typename... ArgTypes> void emplace(ArgTypes &&... Args) {<br>
- reset();<br>
- Storage.hasVal = true;<br>
- new (getPointer()) T(std::forward<ArgTypes>(Args)...);<br>
+ Storage.emplace(std::forward<ArgTypes>(Args)...);<br>
}<br>
<br>
static inline Optional create(const T *y) {<br>
@@ -151,23 +155,17 @@ public:<br>
<br>
void reset() { Storage.reset(); }<br>
<br>
- const T *getPointer() const {<br>
- assert(Storage.hasVal);<br>
- return reinterpret_cast<const T *>(Storage.storage.buffer);<br>
- }<br>
- T *getPointer() {<br>
- assert(Storage.hasVal);<br>
- return reinterpret_cast<T *>(Storage.storage.buffer);<br>
- }<br>
- const T &getValue() const LLVM_LVALUE_FUNCTION { return *getPointer(); }<br>
- T &getValue() LLVM_LVALUE_FUNCTION { return *getPointer(); }<br>
+ const T *getPointer() const { return &Storage.getValue(); }<br>
+ T *getPointer() { return &Storage.getValue(); }<br>
+ const T &getValue() const LLVM_LVALUE_FUNCTION { return Storage.getValue(); }<br>
+ T &getValue() LLVM_LVALUE_FUNCTION { return Storage.getValue(); }<br>
<br>
- explicit operator bool() const { return Storage.hasVal; }<br>
- bool hasValue() const { return Storage.hasVal; }<br>
+ explicit operator bool() const { return hasValue(); }<br>
+ bool hasValue() const { return Storage.hasValue(); }<br>
const T *operator->() const { return getPointer(); }<br>
T *operator->() { return getPointer(); }<br>
- const T &operator*() const LLVM_LVALUE_FUNCTION { return *getPointer(); }<br>
- T &operator*() LLVM_LVALUE_FUNCTION { return *getPointer(); }<br>
+ const T &operator*() const LLVM_LVALUE_FUNCTION { return getValue(); }<br>
+ T &operator*() LLVM_LVALUE_FUNCTION { return getValue(); }<br>
<br>
template <typename U><br>
constexpr T getValueOr(U &&value) const LLVM_LVALUE_FUNCTION {<br>
@@ -175,8 +173,8 @@ public:<br>
}<br>
<br>
#if LLVM_HAS_RVALUE_REFERENCE_THIS<br>
- T &&getValue() && { return std::move(*getPointer()); }<br>
- T &&operator*() && { return std::move(*getPointer()); }<br>
+ T &&getValue() && { return std::move(Storage).getValue(); }<br>
+ T &&operator*() && { return std::move(Storage).getValue(); }<br>
<br>
template <typename U><br>
T getValueOr(U &&value) && {<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>