[llvm] r317019 - [ADT] Split optional to only include copy mechanics and dtor for non-trivial types.
Eric Fiselier via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 17 18:30:26 PST 2018
@Nakamura,
Do you recall more about the build errors you were seeing?
I can't reproduce with GCC 4.8.5 building LLVM and Clang.
Maybe we could try re-committing this patch? At least to rediscover the
exact error.
/Eric
On Wed, Nov 1, 2017 at 4:56 AM, NAKAMURA Takumi via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Excuse me, I have reverted it in r317077.
> It confused clang with g++-4.8. See;
>
> http://bb9.pgr.jp/#/builders/2/builds/335
> http://lab.llvm.org:8011/builders/clang-with-thin-lto-ubuntu/builds/6326
> http://lab.llvm.org:8011/builders/clang-atom-d525-
> fedora-rel/builds/10853
>
>
> On Wed, Nov 1, 2017 at 3:36 AM Benjamin Kramer via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: d0k
>> Date: Tue Oct 31 11:35:54 2017
>> New Revision: 317019
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=317019&view=rev
>> Log:
>> [ADT] Split optional to only include copy mechanics and dtor for
>> non-trivial types.
>>
>> This makes uses of Optional more transparent to the compiler (and
>> clang-tidy) and generates slightly smaller code.
>>
>> Modified:
>> llvm/trunk/include/llvm/ADT/Optional.h
>> llvm/trunk/unittests/ADT/OptionalTest.cpp
>>
>> Modified: llvm/trunk/include/llvm/ADT/Optional.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/
>> llvm/ADT/Optional.h?rev=317019&r1=317018&r2=317019&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/include/llvm/ADT/Optional.h (original)
>> +++ llvm/trunk/include/llvm/ADT/Optional.h Tue Oct 31 11:35:54 2017
>> @@ -27,114 +27,164 @@
>>
>> namespace llvm {
>>
>> -template<typename T>
>> -class Optional {
>> +namespace optional_detail {
>> +/// Storage for any type.
>> +template <typename T, bool IsPodLike> struct OptionalStorage {
>> AlignedCharArrayUnion<T> storage;
>> bool hasVal = false;
>>
>> -public:
>> - using value_type = T;
>> -
>> - Optional(NoneType) {}
>> - explicit Optional() {}
>> -
>> - Optional(const T &y) : hasVal(true) {
>> - new (storage.buffer) T(y);
>> - }
>> + OptionalStorage() = default;
>>
>> - Optional(const Optional &O) : hasVal(O.hasVal) {
>> + 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);
>> + new (storage.buffer) T(*O.getPointer());
>> }
>> -
>> - Optional(T &&y) : hasVal(true) {
>> + OptionalStorage(T &&y) : hasVal(true) {
>> new (storage.buffer) T(std::forward<T>(y));
>> }
>> -
>> - Optional(Optional<T> &&O) : hasVal(O) {
>> - if (O) {
>> - new (storage.buffer) T(std::move(*O));
>> + OptionalStorage(OptionalStorage &&O) : hasVal(O.hasVal) {
>> + if (O.hasVal) {
>> + new (storage.buffer) T(std::move(*O.getPointer()));
>> O.reset();
>> }
>> }
>>
>> - ~Optional() {
>> - reset();
>> - }
>> -
>> - Optional &operator=(T &&y) {
>> + OptionalStorage &operator=(T &&y) {
>> if (hasVal)
>> - **this = std::move(y);
>> + *getPointer() = std::move(y);
>> else {
>> new (storage.buffer) T(std::move(y));
>> hasVal = true;
>> }
>> return *this;
>> }
>> -
>> - Optional &operator=(Optional &&O) {
>> - if (!O)
>> + OptionalStorage &operator=(OptionalStorage &&O) {
>> + if (!O.hasVal)
>> reset();
>> else {
>> - *this = std::move(*O);
>> + *this = std::move(*O.getPointer());
>> O.reset();
>> }
>> return *this;
>> }
>>
>> - /// Create a new object by constructing it in place with the given
>> arguments.
>> - template<typename ...ArgTypes>
>> - void emplace(ArgTypes &&...Args) {
>> - reset();
>> - hasVal = true;
>> - new (storage.buffer) T(std::forward<ArgTypes>(Args)...);
>> - }
>> -
>> - static inline Optional create(const T* y) {
>> - return y ? Optional(*y) : Optional();
>> - }
>> -
>> // 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.
>> - Optional &operator=(const T &y) {
>> + OptionalStorage &operator=(const T &y) {
>> if (hasVal)
>> - **this = y;
>> + *getPointer() = y;
>> else {
>> new (storage.buffer) T(y);
>> hasVal = true;
>> }
>> return *this;
>> }
>> -
>> - Optional &operator=(const Optional &O) {
>> - if (!O)
>> + OptionalStorage &operator=(const OptionalStorage &O) {
>> + if (!O.hasVal)
>> reset();
>> else
>> - *this = *O;
>> + *this = *O.getPointer();
>> return *this;
>> }
>>
>> + ~OptionalStorage() { reset(); }
>> +
>> void reset() {
>> if (hasVal) {
>> - (**this).~T();
>> + (*getPointer()).~T();
>> hasVal = false;
>> }
>> }
>>
>> - const T* getPointer() const { assert(hasVal); return
>> reinterpret_cast<const T*>(storage.buffer); }
>> - T* getPointer() { assert(hasVal); return reinterpret_cast<T*>(storage.buffer);
>> }
>> - const T& getValue() const LLVM_LVALUE_FUNCTION { assert(hasVal);
>> return *getPointer(); }
>> - T& getValue() LLVM_LVALUE_FUNCTION { assert(hasVal); return
>> *getPointer(); }
>> + T *getPointer() {
>> + assert(hasVal);
>> + return reinterpret_cast<T *>(storage.buffer);
>> + }
>> + const T *getPointer() const {
>> + assert(hasVal);
>> + return reinterpret_cast<const T *>(storage.buffer);
>> + }
>> +};
>> +
>> +/// Storage for trivially copyable types only.
>> +template <typename T> struct OptionalStorage<T, true> {
>> + AlignedCharArrayUnion<T> storage;
>> + bool hasVal = false;
>> +
>> + OptionalStorage() = default;
>> +
>> + OptionalStorage(const T &y) : hasVal(true) { new (storage.buffer)
>> T(y); }
>> + OptionalStorage &operator=(const T &y) {
>> + new (storage.buffer) T(y);
>> + hasVal = true;
>> + return *this;
>> + }
>> +
>> + void reset() { hasVal = false; }
>> +};
>> +} // namespace optional_detail
>> +
>> +template <typename T> class Optional {
>> + optional_detail::OptionalStorage<T, isPodLike<T>::value> Storage;
>> +
>> +public:
>> + using value_type = T;
>> +
>> + constexpr Optional() {}
>> + constexpr Optional(NoneType) {}
>> +
>> + Optional(const T &y) : Storage(y) {}
>> + Optional(const Optional &O) = default;
>> +
>> + Optional(T &&y) : Storage(std::forward<T>(y)) {}
>> + Optional(Optional &&O) = default;
>> +
>> + Optional &operator=(T &&y) {
>> + Storage = std::move(y);
>> + return *this;
>> + }
>> + Optional &operator=(Optional &&O) = default;
>> +
>> + /// 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)...);
>> + }
>> +
>> + static inline Optional create(const T *y) {
>> + return y ? Optional(*y) : Optional();
>> + }
>> +
>> + Optional &operator=(const T &y) {
>> + Storage = y;
>> + return *this;
>> + }
>> + Optional &operator=(const Optional &O) = default;
>> +
>> + 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(); }
>>
>> - explicit operator bool() const { return hasVal; }
>> - bool hasValue() const { return hasVal; }
>> + 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 { assert(hasVal);
>> return *getPointer(); }
>> - T& operator*() LLVM_LVALUE_FUNCTION { assert(hasVal); return
>> *getPointer(); }
>> + const T &operator*() const LLVM_LVALUE_FUNCTION { return
>> *getPointer(); }
>> + T &operator*() LLVM_LVALUE_FUNCTION { return *getPointer(); }
>>
>> template <typename U>
>> constexpr T getValueOr(U &&value) const LLVM_LVALUE_FUNCTION {
>> @@ -142,8 +192,8 @@ public:
>> }
>>
>> #if LLVM_HAS_RVALUE_REFERENCE_THIS
>> - T&& getValue() && { assert(hasVal); return std::move(*getPointer()); }
>> - T&& operator*() && { assert(hasVal); return std::move(*getPointer()); }
>> + T &&getValue() && { return std::move(*getPointer()); }
>> + T &&operator*() && { return std::move(*getPointer()); }
>>
>> template <typename U>
>> T getValueOr(U &&value) && {
>>
>> Modified: llvm/trunk/unittests/ADT/OptionalTest.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/
>> ADT/OptionalTest.cpp?rev=317019&r1=317018&r2=317019&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/unittests/ADT/OptionalTest.cpp (original)
>> +++ llvm/trunk/unittests/ADT/OptionalTest.cpp Tue Oct 31 11:35:54 2017
>> @@ -518,5 +518,14 @@ TEST_F(OptionalTest, OperatorGreaterEqua
>> CheckRelation<GreaterEqual>(InequalityLhs, InequalityRhs, !IsLess);
>> }
>>
>> +#if (__has_feature(is_trivially_copyable) && defined(_LIBCPP_VERSION))
>> || \
>> + (defined(__GNUC__) && __GNUC__ >= 5)
>> +static_assert(std::is_trivially_copyable<Optional<int>>::value,
>> + "Should be trivially copyable");
>> +static_assert(
>> + !std::is_trivially_copyable<Optional<NonDefaultConstructible>>::
>> value,
>> + "Shouldn't be trivially copyable");
>> +#endif
>> +
>> } // end anonymous namespace
>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://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/20180117/2ca8b771/attachment.html>
More information about the llvm-commits
mailing list