[llvm] r354217 - Revert [NFC] Better encapsulation of llvm::Optional Storage

[David Blaikie via llvm-commits]

On Sun, Feb 17, 2019 at 6:58 AM Serge Guelton via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> Author: serge_sans_paille
> Date: Sun Feb 17 06:59:21 2019
> New Revision: 354217
>
> URL: http://llvm.org/viewvc/llvm-project?rev=354217&view=rev
> Log:
> Revert  [NFC] Better encapsulation of llvm::Optional Storage
>
> I'm getting the feealing that current Optional implementation is full of
> UB :-/
>

Any particular things worth writing down (in comments and/or commit
messages, etc) for posterity/other people who might stumble across them?


>
> 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=354217&r1=354216&r2=354217&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/ADT/Optional.h (original)
> +++ llvm/trunk/include/llvm/ADT/Optional.h Sun Feb 17 06:59:21 2019
> @@ -30,12 +30,10 @@ class raw_ostream;
>
>  namespace optional_detail {
>  /// Storage for any type.
> -template <typename T, bool = is_trivially_copyable<T>::value>
> -class OptionalStorage {
> +template <typename T, bool = is_trivially_copyable<T>::value> struct
> OptionalStorage {
>    AlignedCharArrayUnion<T> storage;
>    bool hasVal = false;
>
> -public:
>    OptionalStorage() = default;
>
>    OptionalStorage(const T &y) : hasVal(true) { new (storage.buffer) T(y);
> }
> @@ -109,14 +107,6 @@ public:
>      assert(hasVal);
>      return reinterpret_cast<const T *>(storage.buffer);
>    }
> -
> -  template <typename... ArgTypes> void emplace(ArgTypes &&... Args) {
> -    reset();
> -    hasVal = true;
> -    new (storage.buffer) T(std::forward<ArgTypes>(Args)...);
> -  }
> -
> -  bool hasValue() const { return hasVal; }
>  };
>
>  } // namespace optional_detail
> @@ -144,7 +134,9 @@ public:
>
>    /// Create a new object by constructing it in place with the given
> arguments.
>    template <typename... ArgTypes> void emplace(ArgTypes &&... Args) {
> -    Storage.emplace(std::forward<ArgTypes>(Args)...);
> +    reset();
> +    Storage.hasVal = true;
> +    new (getPointer()) T(std::forward<ArgTypes>(Args)...);
>    }
>
>    static inline Optional create(const T *y) {
> @@ -159,13 +151,19 @@ public:
>
>    void reset() { Storage.reset(); }
>
> -  const T *getPointer() const { return Storage.getPointer(); }
> -  T *getPointer() { return Storage.getPointer(); }
> +  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(); }
>
> -  explicit operator bool() const { return hasValue(); }
> -  bool hasValue() const { return Storage.hasValue(); }
> +  explicit operator bool() const { return Storage.hasVal; }
> +  bool hasValue() const { return Storage.hasVal; }
>    const T *operator->() const { return getPointer(); }
>    T *operator->() { return getPointer(); }
>    const T &operator*() const LLVM_LVALUE_FUNCTION { return *getPointer();
> }
>
>
> _______________________________________________
> 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/28c4f1cc/attachment.html>