Merge pull request #2714 from secondlife/nat/xcode-16

Clean up llpointer.h per previous discussions.

11 files changed, 157 insertions, 118 deletions

diff --git a/indra/llappearance/lltexlayerparams.h b/indra/llappearance/lltexlayerparams.h
index 5e785e4f3e..116eb3bee0 100644
--- a/indra/llappearance/lltexlayerparams.h
+++ b/indra/llappearance/lltexlayerparams.h
@@ -30,6 +30,7 @@
 #include "llpointer.h"
 #include "v4color.h"
 #include "llviewervisualparam.h"
+#include <atomic>
 
 class LLAvatarAppearance;
 class LLImageRaw;
diff --git a/indra/llcommon/CMakeLists.txt b/indra/llcommon/CMakeLists.txt
index 78bfaade55..aa8810f00b 100644
--- a/indra/llcommon/CMakeLists.txt
+++ b/indra/llcommon/CMakeLists.txt
@@ -70,6 +70,7 @@ set(llcommon_SOURCE_FILES
     llmetrics.cpp
     llmortician.cpp
     llmutex.cpp
+    llpointer.cpp
     llpredicate.cpp
     llprocess.cpp
     llprocessor.cpp
diff --git a/indra/llcommon/llerror.h b/indra/llcommon/llerror.h
index b17b9ff21e..b6d560a121 100644
--- a/indra/llcommon/llerror.h
+++ b/indra/llcommon/llerror.h
@@ -239,17 +239,12 @@ namespace LLError
 
         ~CallSite();
 
-#ifdef LL_LIBRARY_INCLUDE
-        bool shouldLog();
-#else // LL_LIBRARY_INCLUDE
         bool shouldLog()
         {
             return mCached
                     ? mShouldLog
                     : Log::shouldLog(*this);
         }
-            // this member function needs to be in-line for efficiency
-#endif // LL_LIBRARY_INCLUDE
 
         void invalidate();
 
diff --git a/indra/llcommon/llpointer.cpp b/indra/llcommon/llpointer.cpp
new file mode 100755
index 0000000000..adea447caa
--- /dev/null
+++ b/indra/llcommon/llpointer.cpp
@@ -0,0 +1,26 @@
+/**
+ * @file   llpointer.cpp
+ * @author Nat Goodspeed
+ * @date   2024-09-26
+ * @brief  Implementation for llpointer.
+ *
+ * $LicenseInfo:firstyear=2024&license=viewerlgpl$
+ * Copyright (c) 2024, Linden Research, Inc.
+ * $/LicenseInfo$
+ */
+
+// Precompiled header
+#include "linden_common.h"
+// associated header
+#include "llpointer.h"
+// STL headers
+// std headers
+// external library headers
+// other Linden headers
+#include "llerror.h"
+
+void LLPointerBase::wild_dtor(std::string_view msg)
+{
+//  LL_WARNS() << msg << LL_ENDL;
+    llassert_msg(false, msg);
+}
diff --git a/indra/llcommon/llpointer.h b/indra/llcommon/llpointer.h
index 71c955c4c5..b53cfcdd1a 100644
--- a/indra/llcommon/llpointer.h
+++ b/indra/llcommon/llpointer.h
@@ -26,8 +26,8 @@
 #ifndef LLPOINTER_H
 #define LLPOINTER_H
 
-#include "llerror.h"    // *TODO: consider eliminating this
-#include "llmutex.h"
+#include <boost/functional/hash.hpp>
+#include <string_view>
 #include <utility>                  // std::swap()
 
 //----------------------------------------------------------------------------
@@ -43,8 +43,18 @@
 
 //----------------------------------------------------------------------------
 
+class LLPointerBase
+{
+protected:
+    // alert the coder that a referenced type's destructor did something very
+    // strange -- this is in a non-template base class so we can hide the
+    // implementation in llpointer.cpp
+    static void wild_dtor(std::string_view msg);
+};
+
 // Note: relies on Type having ref() and unref() methods
-template <class Type> class LLPointer
+template <class Type>
+class LLPointer: public LLPointerBase
 {
 public:
     template<typename Subclass>
@@ -106,7 +116,6 @@ public:
     const Type& operator*() const               { return *mPointer; }
     Type&   operator*()                         { 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); }
@@ -189,10 +198,6 @@ public:
     }
 
 protected:
-#ifdef LL_LIBRARY_INCLUDE
-    void ref();
-    void unref();
-#else
     void ref()
     {
         if (mPointer)
@@ -210,12 +215,11 @@ protected:
             temp->unref();
             if (mPointer != nullptr)
             {
-                LL_WARNS() << "Unreference did assignment to non-NULL because of destructor" << LL_ENDL;
+                wild_dtor("Unreference did assignment to non-NULL because of destructor");
                 unref();
             }
         }
     }
-#endif // LL_LIBRARY_INCLUDE
 
 protected:
     Type*   mPointer;
diff --git a/indra/llcommon/owning_ptr.h b/indra/llcommon/owning_ptr.h
new file mode 100755
index 0000000000..7cf8d3f0ba
--- /dev/null
+++ b/indra/llcommon/owning_ptr.h
@@ -0,0 +1,71 @@
+/**
+ * @file   owning_ptr.h
+ * @author Nat Goodspeed
+ * @date   2024-09-27
+ * @brief  owning_ptr<T> is like std::unique_ptr<T>, but easier to integrate
+ *
+ * $LicenseInfo:firstyear=2024&license=viewerlgpl$
+ * Copyright (c) 2024, Linden Research, Inc.
+ * $/LicenseInfo$
+ */
+
+#if ! defined(LL_OWNING_PTR_H)
+#define LL_OWNING_PTR_H
+
+#include <functional>
+#include <memory>
+
+/**
+ * owning_ptr<T> adapts std::unique_ptr<T> to make it easier to adopt into
+ * older code using dumb pointers.
+ *
+ * Consider a class Outer with a member Thing* mThing. After the constructor,
+ * each time a method wants to assign to mThing, it must test for nullptr and
+ * destroy the previous Thing instance. During Outer's lifetime, mThing is
+ * passed to legacy domain-specific functions accepting plain Thing*. Finally
+ * the destructor must again test for nullptr and destroy the remaining Thing
+ * instance.
+ *
+ * Multiply that by several different Outer members of different types,
+ * possibly with different domain-specific destructor functions.
+ *
+ * Dropping std::unique_ptr<Thing> into Outer is cumbersome for a several
+ * reasons. First, if Thing requires a domain-specific destructor function,
+ * the unique_ptr declaration of mThing must explicitly state the type of that
+ * function (as a function pointer, for a typical legacy function). Second,
+ * every Thing* assignment to mThing must be changed to mThing.reset(). Third,
+ * every time we call a legacy domain-specific function, we must pass
+ * mThing.get().
+ *
+ * owning_ptr<T> is designed to drop into a situation like this. The domain-
+ * specific destructor function, if any, is passed to its constructor; it need
+ * not be encoded into the pointer type. owning_ptr<T> supports plain pointer
+ * assignment, internally calling std::unique_ptr<T>::reset(). It also
+ * supports implicit conversion to plain T*, to pass the owned pointer to
+ * legacy domain-specific functions.
+ *
+ * Obviously owning_ptr<T> must not be used in situations where ownership of
+ * the referenced object is passed on to another pointer: use std::unique_ptr
+ * for that. Similarly, it is not for shared ownership. It simplifies lifetime
+ * management for classes that currently store (and explicitly destroy) plain
+ * T* pointers.
+ */
+template <typename T>
+class owning_ptr
+{
+    using deleter = std::function<void(T*)>;
+public:
+    owning_ptr(T* p=nullptr, const deleter& d=std::default_delete<T>()):
+        mPtr(p, d)
+    {}
+    void reset(T* p=nullptr) { mPtr.reset(p); }
+    owning_ptr& operator=(T* p) { mPtr.reset(p); return *this; }
+    operator T*() const { return mPtr.get(); }
+    T& operator*() const { return *mPtr; }
+    T* operator->() const { return mPtr.operator->(); }
+
+private:
+    std::unique_ptr<T, deleter> mPtr;
+};
+
+#endif /* ! defined(LL_OWNING_PTR_H) */
diff --git a/indra/llimage/llimage.h b/indra/llimage/llimage.h
index 6b14b68c78..1fb61673bd 100644
--- a/indra/llimage/llimage.h
+++ b/indra/llimage/llimage.h
@@ -27,10 +27,11 @@
 #ifndef LL_LLIMAGE_H
 #define LL_LLIMAGE_H
 
-#include "lluuid.h"
-#include "llstring.h"
+#include "llmutex.h"
 #include "llpointer.h"
+#include "llstring.h"
 #include "lltrace.h"
+#include "lluuid.h"
 
 constexpr S32 MIN_IMAGE_MIP =  2; // 4x4, only used for expand/contract power of 2
 constexpr S32 MAX_IMAGE_MIP = 12; // 4096x4096
diff --git a/indra/llimagej2coj/llimagej2coj.cpp b/indra/llimagej2coj/llimagej2coj.cpp
index 6037517103..d8f17561b3 100644
--- a/indra/llimagej2coj/llimagej2coj.cpp
+++ b/indra/llimagej2coj/llimagej2coj.cpp
@@ -31,6 +31,8 @@
 #include "openjpeg.h"
 #include "event.h"
 #include "cio.h"
+#include "owning_ptr.h"
+#include <string>
 
 #define MAX_ENCODED_DISCARD_LEVELS 5
 
@@ -231,33 +233,6 @@ public:
         parameters.cp_reduce = discardLevel;
     }
 
-    ~JPEG2KDecode()
-    {
-        if (decoder)
-        {
-            opj_destroy_codec(decoder);
-        }
-        decoder = nullptr;
-
-        if (image)
-        {
-            opj_image_destroy(image);
-        }
-        image = nullptr;
-
-        if (stream)
-        {
-            opj_stream_destroy(stream);
-        }
-        stream = nullptr;
-
-        if (codestream_info)
-        {
-            opj_destroy_cstr_info(&codestream_info);
-        }
-        codestream_info = nullptr;
-    }
-
     bool readHeader(
         U8* data,
         U32 dataSize,
@@ -275,11 +250,6 @@ public:
             return false;
         }
 
-        if (stream)
-        {
-            opj_stream_destroy(stream);
-        }
-
         stream = opj_stream_create(dataSize, true);
         if (!stream)
         {
@@ -301,13 +271,14 @@ public:
         opj_decoder_set_strict_mode(decoder, OPJ_FALSE);
 
         /* Read the main header of the codestream and if necessary the JP2 boxes*/
-        if (!opj_read_header((opj_stream_t*)stream, decoder, &image))
+        opj_image_t* img;
+        if (!opj_read_header(stream, decoder, &img))
         {
             return false;
         }
+        image = img;
 
         codestream_info = opj_get_cstr_info(decoder);
-
         if (!codestream_info)
         {
             return false;
@@ -344,11 +315,6 @@ public:
         opj_set_warning_handler(decoder, opj_warn, this);
         opj_set_error_handler(decoder, opj_error, this);
 
-        if (stream)
-        {
-            opj_stream_destroy(stream);
-        }
-
         stream = opj_stream_create(dataSize, true);
         if (!stream)
         {
@@ -366,11 +332,7 @@ public:
         size = dataSize;
         offset = 0;
 
-        if (image)
-        {
-            opj_image_destroy(image);
-            image = nullptr;
-        }
+        image = nullptr;
 
         // needs to happen before opj_read_header and opj_decode...
         opj_set_decoded_resolution_factor(decoder, discard_level);
@@ -378,10 +340,12 @@ public:
         // enable decoding partially loaded images
         opj_decoder_set_strict_mode(decoder, OPJ_FALSE);
 
-        if (!opj_read_header(stream, decoder, &image))
+        opj_image_t* img;
+        if (!opj_read_header(stream, decoder, &img))
         {
             return false;
         }
+        image = img;
 
         // needs to happen before decode which may fail
         if (channels)
@@ -393,15 +357,9 @@ public:
 
         // count was zero.  The latter is just a sanity check before we
         // dereference the array.
-        if (!decoded || !image || !image->numcomps)
-        {
-            opj_end_decompress(decoder, stream);
-            return false;
-        }
-
+        bool result = (decoded && image && image->numcomps);
         opj_end_decompress(decoder, stream);
-
-        return true;
+        return result;
     }
 
     opj_image_t* getImage() { return image; }
@@ -409,10 +367,18 @@ public:
 private:
     opj_dparameters_t         parameters;
     opj_event_mgr_t           event_mgr;
-    opj_image_t*              image = nullptr;
-    opj_codec_t*              decoder = nullptr;
-    opj_stream_t*             stream = nullptr;
-    opj_codestream_info_v2_t* codestream_info = nullptr;
+    owning_ptr<opj_codestream_info_v2_t> codestream_info{
+        nullptr,
+        // opj_destroy_cstr_info(opj_codestream_info_v2_t**) requires a
+        // pointer to pointer, which is too bad because otherwise we could
+        // directly pass that function as the owning_ptr's deleter.
+        [](opj_codestream_info_v2_t* doomed)
+        {
+            opj_destroy_cstr_info(&doomed);
+        }};
+    owning_ptr<opj_stream_t>  stream{ nullptr, opj_stream_destroy };
+    owning_ptr<opj_image_t>   image{ nullptr, opj_image_destroy };
+    owning_ptr<opj_codec_t>   decoder{ nullptr, opj_destroy_codec };
 };
 
 class JPEG2KEncode : public JPEG2KBase
@@ -447,43 +413,19 @@ public:
             parameters.irreversible = 1;
         }
 
-        if (comment_text)
-        {
-            free(comment_text);
-        }
-        comment_text = comment_text_in ? strdup(comment_text_in) : nullptr;
+        comment_text.assign(comment_text_in? comment_text_in : "no comment");
 
-        parameters.cp_comment = comment_text ? comment_text : (char*)"no comment";
+        // Because comment_text is a member declared before parameters,
+        // it will outlive parameters, so we can safely store in parameters a
+        // pointer into comment_text's data. Unfortunately cp_comment is
+        // declared as (non-const) char*. We just have to trust that this is
+        // legacy C style coding, rather than any intention to modify the
+        // comment string. (If there was actual modification, we could use a
+        // std::vector<char> instead, but let's only go there if we must.)
+        parameters.cp_comment = const_cast<char*>(comment_text.c_str());
         llassert(parameters.cp_comment);
     }
 
-    ~JPEG2KEncode()
-    {
-        if (encoder)
-        {
-            opj_destroy_codec(encoder);
-        }
-        encoder = nullptr;
-
-        if (image)
-        {
-            opj_image_destroy(image);
-        }
-        image = nullptr;
-
-        if (stream)
-        {
-            opj_stream_destroy(stream);
-        }
-        stream = nullptr;
-
-        if (comment_text)
-        {
-            free(comment_text);
-        }
-        comment_text = nullptr;
-    }
-
     bool encode(const LLImageRaw& rawImageIn, LLImageJ2C &compressedImageOut)
     {
         LLImageDataSharedLock lockIn(&rawImageIn);
@@ -561,11 +503,6 @@ public:
 
         memset(buffer, 0, data_size_guess);
 
-        if (stream)
-        {
-            opj_stream_destroy(stream);
-        }
-
         stream = opj_stream_create(data_size_guess, false);
         if (!stream)
         {
@@ -742,12 +679,12 @@ public:
     opj_image_t* getImage() { return image; }
 
 private:
-    opj_cparameters_t   parameters;
-    opj_event_mgr_t     event_mgr;
-    opj_image_t*        image = nullptr;
-    opj_codec_t*        encoder = nullptr;
-    opj_stream_t*       stream = nullptr;
-    char*               comment_text = nullptr;
+    std::string              comment_text;
+    opj_cparameters_t        parameters;
+    opj_event_mgr_t          event_mgr;
+    owning_ptr<opj_stream_t> stream{ nullptr,  opj_stream_destroy };
+    owning_ptr<opj_image_t>  image{ nullptr,   opj_image_destroy };
+    owning_ptr<opj_codec_t>  encoder{ nullptr, opj_destroy_codec };
 };
 
 
diff --git a/indra/llmath/llvolumemgr.cpp b/indra/llmath/llvolumemgr.cpp
index bb0c94d513..d8f649140f 100644
--- a/indra/llmath/llvolumemgr.cpp
+++ b/indra/llmath/llvolumemgr.cpp
@@ -25,6 +25,7 @@
 
 #include "linden_common.h"
 
+#include "llmutex.h"
 #include "llvolumemgr.h"
 #include "llvolume.h"
 
diff --git a/indra/llui/llnotifications.h b/indra/llui/llnotifications.h
index 9b83da13ad..9d59b99349 100644
--- a/indra/llui/llnotifications.h
+++ b/indra/llui/llnotifications.h
@@ -94,6 +94,7 @@
 #include "llinitparam.h"
 #include "llinstancetracker.h"
 #include "llmortician.h"
+#include "llmutex.h"
 #include "llnotificationptr.h"
 #include "llpointer.h"
 #include "llrefcount.h"
diff --git a/indra/newview/llwatchdog.cpp b/indra/newview/llwatchdog.cpp
index bf171fe954..d9fcd5811d 100644
--- a/indra/newview/llwatchdog.cpp
+++ b/indra/newview/llwatchdog.cpp
@@ -27,6 +27,7 @@
 
 #include "llviewerprecompiledheaders.h"
 #include "llwatchdog.h"
+#include "llmutex.h"
 #include "llthread.h"
 
 constexpr U32 WATCHDOG_SLEEP_TIME_USEC = 1000000U;