<div dir="ltr">Hey Ben,<div><br></div><div>This change broke some clangd code (no failing test, mea culpa), because it changed the move semantics.</div><div><br></div><div>Previously: a moved-from llvm::Optional was None (for all types).</div><div>Now: a moved-from llvm::Optional is None (for non-trivial types), and has the old value (for trivial types).</div><div><br></div><div>FWIW, a moved-from std::optional is *not* nullopt, and contains the moved-from value.</div><div>This seems sad to me, and kills a nice use of optional (<a href="https://reviews.llvm.org/source/clang-tools-extra/browse/clang-tools-extra/trunk/clangd/Function.h;323350$162">this one</a>), but there's some value in consistency with std.</div><div><br></div><div>Either way, I wanted to bring this up because</div><div> - I wasn't sure it was explicitly considered, and should probably be documented</div><div> - I think we should have either the old llvm behavior or the std behavior, the new hybrid is a gotcha I think.</div><div><br></div><div>WDYT?</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 18, 2018 at 12:26 PM, Benjamin Kramer via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: d0k<br>
Date: Thu Jan 18 03:26:24 2018<br>
New Revision: 322838<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=322838&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=322838&view=rev</a><br>
Log:<br>
[ADT] Split optional to only include copy mechanics and dtor for non-trivial types.<br>
<br>
This makes uses of Optional more transparent to the compiler (and<br>
clang-tidy) and generates slightly smaller code.<br>
<br>
This is a re-land of r317019, which had issues with GCC 4.8 back then.<br>
Those issues don't reproduce anymore, but I'll watch the buildbots<br>
closely in case anything goes wrong.<br>
<br>
Modified:<br>
llvm/trunk/include/llvm/ADT/<wbr>Optional.h<br>
llvm/trunk/unittests/ADT/<wbr>OptionalTest.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/ADT/<wbr>Optional.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/Optional.h?rev=322838&r1=322837&r2=322838&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/include/<wbr>llvm/ADT/Optional.h?rev=<wbr>322838&r1=322837&r2=322838&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/include/llvm/ADT/<wbr>Optional.h (original)<br>
+++ llvm/trunk/include/llvm/ADT/<wbr>Optional.h Thu Jan 18 03:26:24 2018<br>
@@ -27,139 +27,173 @@<br>
<br>
namespace llvm {<br>
<br>
-template <typename T> class Optional {<br>
+namespace optional_detail {<br>
+/// Storage for any type.<br>
+template <typename T, bool IsPodLike> struct OptionalStorage {<br>
AlignedCharArrayUnion<T> storage;<br>
bool hasVal = false;<br>
<br>
-public:<br>
- using value_type = T;<br>
-<br>
- Optional(NoneType) {}<br>
- explicit Optional() {}<br>
-<br>
- Optional(const T &y) : hasVal(true) { new (storage.buffer) T(y); }<br>
+ OptionalStorage() = default;<br>
<br>
- Optional(const Optional &O) : hasVal(O.hasVal) {<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);<br>
+ new (storage.buffer) T(*O.getPointer());<br>
}<br>
-<br>
- Optional(T &&y) : hasVal(true) { new (storage.buffer) T(std::forward<T>(y)); }<br>
-<br>
- Optional(Optional<T> &&O) : hasVal(O) {<br>
- if (O) {<br>
- new (storage.buffer) T(std::move(*O));<br>
+ OptionalStorage(T &&y) : hasVal(true) {<br>
+ new (storage.buffer) T(std::forward<T>(y));<br>
+ }<br>
+ OptionalStorage(<wbr>OptionalStorage &&O) : hasVal(O.hasVal) {<br>
+ if (O.hasVal) {<br>
+ new (storage.buffer) T(std::move(*O.getPointer()));<br>
O.reset();<br>
}<br>
}<br>
<br>
- ~Optional() { reset(); }<br>
-<br>
- Optional &operator=(T &&y) {<br>
+ OptionalStorage &operator=(T &&y) {<br>
if (hasVal)<br>
- **this = std::move(y);<br>
+ *getPointer() = std::move(y);<br>
else {<br>
new (storage.buffer) T(std::move(y));<br>
hasVal = true;<br>
}<br>
return *this;<br>
}<br>
-<br>
- Optional &operator=(Optional &&O) {<br>
- if (!O)<br>
+ OptionalStorage &operator=(OptionalStorage &&O) {<br>
+ if (!O.hasVal)<br>
reset();<br>
else {<br>
- *this = std::move(*O);<br>
+ *this = std::move(*O.getPointer());<br>
O.reset();<br>
}<br>
return *this;<br>
}<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>
- hasVal = true;<br>
- new (storage.buffer) T(std::forward<ArgTypes>(Args)<wbr>...);<br>
- }<br>
-<br>
- static inline Optional create(const T *y) {<br>
- return y ? Optional(*y) : Optional();<br>
- }<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>
- Optional &operator=(const T &y) {<br>
+ OptionalStorage &operator=(const T &y) {<br>
if (hasVal)<br>
- **this = y;<br>
+ *getPointer() = y;<br>
else {<br>
new (storage.buffer) T(y);<br>
hasVal = true;<br>
}<br>
return *this;<br>
}<br>
-<br>
- Optional &operator=(const Optional &O) {<br>
- if (!O)<br>
+ OptionalStorage &operator=(const OptionalStorage &O) {<br>
+ if (!O.hasVal)<br>
reset();<br>
else<br>
- *this = *O;<br>
+ *this = *O.getPointer();<br>
return *this;<br>
}<br>
<br>
+ ~OptionalStorage() { reset(); }<br>
+<br>
void reset() {<br>
if (hasVal) {<br>
- (**this).~T();<br>
+ (*getPointer()).~T();<br>
hasVal = false;<br>
}<br>
}<br>
<br>
- const T *getPointer() const {<br>
- assert(hasVal);<br>
- return reinterpret_cast<const T *>(storage.buffer);<br>
- }<br>
T *getPointer() {<br>
assert(hasVal);<br>
return reinterpret_cast<T *>(storage.buffer);<br>
}<br>
- const T &getValue() const LLVM_LVALUE_FUNCTION {<br>
+ const T *getPointer() const {<br>
assert(hasVal);<br>
- return *getPointer();<br>
+ return reinterpret_cast<const T *>(storage.buffer);<br>
}<br>
- T &getValue() LLVM_LVALUE_FUNCTION {<br>
- assert(hasVal);<br>
- return *getPointer();<br>
+};<br>
+<br>
+/// Storage for trivially copyable types only.<br>
+template <typename T> struct OptionalStorage<T, true> {<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 &operator=(const T &y) {<br>
+ new (storage.buffer) T(y);<br>
+ hasVal = true;<br>
+ return *this;<br>
}<br>
<br>
- explicit operator bool() const { return hasVal; }<br>
- bool hasValue() const { return hasVal; }<br>
- const T *operator->() const { return getPointer(); }<br>
- T *operator->() { return getPointer(); }<br>
- const T &operator*() const LLVM_LVALUE_FUNCTION {<br>
- assert(hasVal);<br>
- return *getPointer();<br>
+ void reset() { hasVal = false; }<br>
+};<br>
+} // namespace optional_detail<br>
+<br>
+template <typename T> class Optional {<br>
+ optional_detail::<wbr>OptionalStorage<T, isPodLike<T>::value> Storage;<br>
+<br>
+public:<br>
+ using value_type = T;<br>
+<br>
+ constexpr Optional() {}<br>
+ constexpr Optional(NoneType) {}<br>
+<br>
+ Optional(const T &y) : Storage(y) {}<br>
+ Optional(const Optional &O) = default;<br>
+<br>
+ Optional(T &&y) : Storage(std::forward<T>(y)) {}<br>
+ Optional(Optional &&O) = default;<br>
+<br>
+ Optional &operator=(T &&y) {<br>
+ Storage = std::move(y);<br>
+ return *this;<br>
}<br>
- T &operator*() LLVM_LVALUE_FUNCTION {<br>
- assert(hasVal);<br>
- return *getPointer();<br>
+ Optional &operator=(Optional &&O) = default;<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)<wbr>...);<br>
}<br>
<br>
+ static inline Optional create(const T *y) {<br>
+ return y ? Optional(*y) : Optional();<br>
+ }<br>
+<br>
+ Optional &operator=(const T &y) {<br>
+ Storage = y;<br>
+ return *this;<br>
+ }<br>
+ Optional &operator=(const Optional &O) = default;<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>
+<br>
+ explicit operator bool() const { return Storage.hasVal; }<br>
+ bool hasValue() const { return Storage.hasVal; }<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>
+<br>
template <typename U><br>
constexpr T getValueOr(U &&value) const LLVM_LVALUE_FUNCTION {<br>
return hasValue() ? getValue() : std::forward<U>(value);<br>
}<br>
<br>
#if LLVM_HAS_RVALUE_REFERENCE_THIS<br>
- T &&getValue() && {<br>
- assert(hasVal);<br>
- return std::move(*getPointer());<br>
- }<br>
- T &&operator*() && {<br>
- assert(hasVal);<br>
- return std::move(*getPointer());<br>
- }<br>
+ T &&getValue() && { return std::move(*getPointer()); }<br>
+ T &&operator*() && { return std::move(*getPointer()); }<br>
<br>
template <typename U><br>
T getValueOr(U &&value) && {<br>
<br>
Modified: llvm/trunk/unittests/ADT/<wbr>OptionalTest.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/OptionalTest.cpp?rev=322838&r1=322837&r2=322838&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/unittests/<wbr>ADT/OptionalTest.cpp?rev=<wbr>322838&r1=322837&r2=322838&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/unittests/ADT/<wbr>OptionalTest.cpp (original)<br>
+++ llvm/trunk/unittests/ADT/<wbr>OptionalTest.cpp Thu Jan 18 03:26:24 2018<br>
@@ -518,5 +518,14 @@ TEST_F(OptionalTest, OperatorGreaterEqua<br>
CheckRelation<GreaterEqual>(<wbr>InequalityLhs, InequalityRhs, !IsLess);<br>
}<br>
<br>
+#if (__has_feature(is_trivially_<wbr>copyable) && defined(_LIBCPP_VERSION)) || \<br>
+ (defined(__GNUC__) && __GNUC__ >= 5)<br>
+static_assert(std::is_<wbr>trivially_copyable<Optional<<wbr>int>>::value,<br>
+ "Should be trivially copyable");<br>
+static_assert(<br>
+ !std::is_trivially_copyable<<wbr>Optional<<wbr>NonDefaultConstructible>>::<wbr>value,<br>
+ "Shouldn't be trivially copyable");<br>
+#endif<br>
+<br>
} // end anonymous namespace<br>
<br>
<br>
<br>
______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div>