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