From 705fd68ed7cdc6db39ddd237b7f2c70045fc01fb Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 24 Sep 2024 12:53:33 -0400 Subject: Make `LLPointer` equality comparisons accept not-identical types. That is, both `LLPointer::operator==()` and `operator!=()` and the free function `operator==()` and `operator!=()` now accept pointer types other than the type of the subject `LLPointer`, letting the compiler apply implicit pointer conversions or diagnose an error. --- indra/llcommon/llpointer.h | 23 ++++++++++++++--------- indra/newview/llviewerwindow.cpp | 2 +- 2 files changed, 15 insertions(+), 10 deletions(-) (limited to 'indra') diff --git a/indra/llcommon/llpointer.h b/indra/llcommon/llpointer.h index 048547e4cc..a886d6538e 100644 --- a/indra/llcommon/llpointer.h +++ b/indra/llcommon/llpointer.h @@ -105,11 +105,16 @@ public: bool notNull() const { return (mPointer != nullptr); } operator Type*() const { return mPointer; } - bool operator !=(Type* ptr) const { return (mPointer != ptr); } - bool operator ==(Type* ptr) const { return (mPointer == ptr); } - bool operator ==(const LLPointer& ptr) const { return (mPointer == ptr.mPointer); } - bool operator < (const LLPointer& ptr) const { return (mPointer < ptr.mPointer); } - bool operator > (const LLPointer& ptr) const { return (mPointer > ptr.mPointer); } + template + bool operator !=(Type1* ptr) const { return (mPointer != ptr); } + template + bool operator ==(Type1* ptr) const { return (mPointer == ptr); } + template + bool operator !=(const LLPointer& ptr) const { return (mPointer != ptr.mPointer); } + template + bool operator ==(const LLPointer& ptr) const { return (mPointer == ptr.mPointer); } + bool operator < (const LLPointer& ptr) const { return (mPointer < ptr.mPointer); } + bool operator > (const LLPointer& ptr) const { return (mPointer > ptr.mPointer); } LLPointer& operator =(Type* ptr) { @@ -418,14 +423,14 @@ private: bool mStayUnique; }; -template -bool operator!=(Type* lhs, const LLPointer& rhs) +template +bool operator!=(Type0* lhs, const LLPointer& rhs) { return (lhs != rhs.get()); } -template -bool operator==(Type* lhs, const LLPointer& rhs) +template +bool operator==(Type0* lhs, const LLPointer& rhs) { return (lhs == rhs.get()); } diff --git a/indra/newview/llviewerwindow.cpp b/indra/newview/llviewerwindow.cpp index d35a504228..7f45e109cb 100644 --- a/indra/newview/llviewerwindow.cpp +++ b/indra/newview/llviewerwindow.cpp @@ -1331,7 +1331,7 @@ LLWindowCallbacks::DragNDropResult LLViewerWindow::handleDragNDrop( LLWindow *wi // Check the whitelist, if there's media (otherwise just show it) if (te->getMediaData() == NULL || te->getMediaData()->checkCandidateUrl(url)) { - if (obj != mDragHoveredObject.get()) + if (obj != mDragHoveredObject) { // Highlight the dragged object LLSelectMgr::getInstance()->unhighlightObjectOnly(mDragHoveredObject); -- cgit v1.2.3 From 1a682de4e42dcea932d25ffe980a2dfec345211e Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Tue, 24 Sep 2024 13:41:02 -0400 Subject: LLConstPointer is the same as LLPointer. Instead of restating the whole class, requiring changes to be made in parallel (which has already failed), just make a template alias. --- indra/llcommon/llpointer.h | 166 +-------------------------------------------- 1 file changed, 2 insertions(+), 164 deletions(-) (limited to 'indra') diff --git a/indra/llcommon/llpointer.h b/indra/llcommon/llpointer.h index a886d6538e..bbe3595435 100644 --- a/indra/llcommon/llpointer.h +++ b/indra/llcommon/llpointer.h @@ -210,170 +210,8 @@ protected: Type* mPointer; }; -template class LLConstPointer -{ - template - friend class LLConstPointer; -public: - LLConstPointer() : - mPointer(nullptr) - { - } - - LLConstPointer(const Type* ptr) : - mPointer(ptr) - { - ref(); - } - - LLConstPointer(const LLConstPointer& ptr) : - mPointer(ptr.mPointer) - { - ref(); - } - - LLConstPointer(LLConstPointer&& ptr) noexcept - { - mPointer = ptr.mPointer; - ptr.mPointer = nullptr; - } - - // support conversion up the type hierarchy. See Item 45 in Effective C++, 3rd Ed. - template - LLConstPointer(const LLConstPointer& ptr) : - mPointer(ptr.get()) - { - ref(); - } - - template - LLConstPointer(LLConstPointer&& ptr) noexcept : - mPointer(ptr.get()) - { - ptr.mPointer = nullptr; - } - - ~LLConstPointer() - { - unref(); - } - - const Type* get() const { return mPointer; } - const Type* operator->() const { return mPointer; } - const Type& operator*() const { return *mPointer; } - - operator BOOL() const { return (mPointer != nullptr); } - operator bool() const { return (mPointer != nullptr); } - bool operator!() const { return (mPointer == nullptr); } - bool isNull() const { return (mPointer == nullptr); } - bool notNull() const { return (mPointer != nullptr); } - - operator const Type*() const { return mPointer; } - bool operator !=(const Type* ptr) const { return (mPointer != ptr); } - bool operator ==(const Type* ptr) const { return (mPointer == ptr); } - bool operator ==(const LLConstPointer& ptr) const { return (mPointer == ptr.mPointer); } - bool operator < (const LLConstPointer& ptr) const { return (mPointer < ptr.mPointer); } - bool operator > (const LLConstPointer& ptr) const { return (mPointer > ptr.mPointer); } - - LLConstPointer& operator =(const Type* ptr) - { - if( mPointer != ptr ) - { - unref(); - mPointer = ptr; - ref(); - } - - return *this; - } - - LLConstPointer& operator =(const LLConstPointer& ptr) - { - if( mPointer != ptr.mPointer ) - { - unref(); - mPointer = ptr.mPointer; - ref(); - } - return *this; - } - - LLConstPointer& operator =(LLConstPointer&& ptr) - { - if (mPointer != ptr.mPointer) - { - unref(); - mPointer = ptr.mPointer; - ptr.mPointer = nullptr; - } - return *this; - } - - // support assignment up the type hierarchy. See Item 45 in Effective C++, 3rd Ed. - template - LLConstPointer& operator =(const LLConstPointer& ptr) - { - if( mPointer != ptr.get() ) - { - unref(); - mPointer = ptr.get(); - ref(); - } - return *this; - } - - template - LLConstPointer& operator =(LLConstPointer&& ptr) - { - if (mPointer != ptr.mPointer) - { - unref(); - mPointer = ptr.mPointer; - ptr.mPointer = nullptr; - } - return *this; - } - - // Just exchange the pointers, which will not change the reference counts. - static void swap(LLConstPointer& a, LLConstPointer& b) - { - const Type* temp = a.mPointer; - a.mPointer = b.mPointer; - b.mPointer = temp; - } - -protected: -#ifdef LL_LIBRARY_INCLUDE - void ref(); - void unref(); -#else // LL_LIBRARY_INCLUDE - void ref() - { - if (mPointer) - { - mPointer->ref(); - } - } - - void unref() - { - if (mPointer) - { - const Type *temp = mPointer; - mPointer = nullptr; - temp->unref(); - if (mPointer != nullptr) - { - LL_WARNS() << "Unreference did assignment to non-NULL because of destructor" << LL_ENDL; - unref(); - } - } - } -#endif // LL_LIBRARY_INCLUDE - -protected: - const Type* mPointer; -}; +template +using LLConstPointer = LLPointer; template class LLCopyOnWritePointer : public LLPointer -- cgit v1.2.3 From 98c829a47cd2b38d51602cce3de873179a394ba7 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 25 Sep 2024 09:43:43 -0400 Subject: Use copy-and-swap idiom for LLPointer's assignment operators. This affords strong exception safety. Also, eliminating the conditional may improve speed. --- indra/llcommon/llpointer.h | 53 ++++++++++++++++++++++------------------------ 1 file changed, 25 insertions(+), 28 deletions(-) (limited to 'indra') diff --git a/indra/llcommon/llpointer.h b/indra/llcommon/llpointer.h index bbe3595435..c5dd5532cc 100644 --- a/indra/llcommon/llpointer.h +++ b/indra/llcommon/llpointer.h @@ -28,6 +28,7 @@ #include "llerror.h" // *TODO: consider eliminating this #include "llmutex.h" +#include // std::swap() //---------------------------------------------------------------------------- // RefCount objects should generally only be accessed by way of LLPointer<>'s @@ -118,24 +119,26 @@ public: LLPointer& operator =(Type* ptr) { - assign(ptr); + // copy-and-swap idiom, see http://gotw.ca/gotw/059.htm + LLPointer temp(ptr); + using std::swap; // per Swappable convention + swap(*this, temp); return *this; } LLPointer& operator =(const LLPointer& ptr) { - assign(ptr); + LLPointer temp(ptr); + using std::swap; // per Swappable convention + swap(*this, temp); return *this; } LLPointer& operator =(LLPointer&& ptr) { - if (mPointer != ptr.mPointer) - { - unref(); - mPointer = ptr.mPointer; - ptr.mPointer = nullptr; - } + LLPointer temp(std::move(ptr)); + using std::swap; // per Swappable convention + swap(*this, temp); return *this; } @@ -143,28 +146,32 @@ public: template LLPointer& operator =(const LLPointer& ptr) { - assign(ptr.get()); + LLPointer temp(ptr); + using std::swap; // per Swappable convention + swap(*this, temp); return *this; } template LLPointer& operator =(LLPointer&& ptr) { - if (mPointer != ptr.mPointer) - { - unref(); - mPointer = ptr.mPointer; - ptr.mPointer = nullptr; - } + LLPointer temp(std::move(ptr)); + using std::swap; // per Swappable convention + swap(*this, temp); return *this; } // Just exchange the pointers, which will not change the reference counts. static void swap(LLPointer& a, LLPointer& b) { - Type* temp = a.mPointer; - a.mPointer = b.mPointer; - b.mPointer = temp; + using std::swap; // per Swappable convention + swap(a.mPointer, b.mPointer); + } + + // Put swap() overload in the global namespace, per Swappable convention + friend void swap(LLPointer& a, LLPointer& b) + { + LLPointer::swap(a, b); } protected: @@ -196,16 +203,6 @@ protected: } #endif // LL_LIBRARY_INCLUDE - void assign(const LLPointer& ptr) - { - if (mPointer != ptr.mPointer) - { - unref(); - mPointer = ptr.mPointer; - ref(); - } - } - protected: Type* mPointer; }; -- cgit v1.2.3 From 5d7b3abe7759545b5ff12dc8ae5a6de9ed258953 Mon Sep 17 00:00:00 2001 From: Nat Goodspeed Date: Wed, 25 Sep 2024 10:15:20 -0400 Subject: Explain why apparently redundant LLPointer methods are necessary. Given templated constructors and assignment operators, it's tempting to remove specific constructors and assignment operators with the same LLPointer parameters. That turns out to be a mistake. Add comments to warn future maintainers. --- indra/llcommon/llpointer.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 'indra') diff --git a/indra/llcommon/llpointer.h b/indra/llcommon/llpointer.h index c5dd5532cc..71c955c4c5 100644 --- a/indra/llcommon/llpointer.h +++ b/indra/llcommon/llpointer.h @@ -61,6 +61,13 @@ public: ref(); } + // Even though the template constructors below accepting + // (const LLPointer&) and (LLPointer&&) appear to + // subsume these specific (const LLPointer&) and (LLPointer&&) + // constructors, the compiler recognizes these as The Copy Constructor and + // The Move Constructor, respectively. In other words, even in the + // presence of the LLPointer constructors, we still must specify + // the LLPointer constructors. LLPointer(const LLPointer& ptr) : mPointer(ptr.mPointer) { @@ -126,6 +133,13 @@ public: return *this; } + // Even though the template assignment operators below accepting + // (const LLPointer&) and (LLPointer&&) appear to + // subsume these specific (const LLPointer&) and (LLPointer&&) + // assignment operators, the compiler recognizes these as Copy Assignment + // and Move Assignment, respectively. In other words, even in the presence + // of the LLPointer assignment operators, we still must specify + // the LLPointer operators. LLPointer& operator =(const LLPointer& ptr) { LLPointer temp(ptr); -- cgit v1.2.3