DRTVWR-476: Review response: remove wait_until() methods and LLDate.

2 files changed, 47 insertions, 229 deletions

diff --git a/indra/llcommon/llcond.cpp b/indra/llcommon/llcond.cpp
deleted file mode 100644
index d5362a48fc..0000000000
--- a/indra/llcommon/llcond.cpp
+++ /dev/null
@@ -1,111 +0,0 @@
-/**
- * @file   llcond.cpp
- * @author Nat Goodspeed
- * @date   2019-07-17
- * @brief  Implementation for llcond.
- * 
- * $LicenseInfo:firstyear=2019&license=viewerlgpl$
- * Copyright (c) 2019, Linden Research, Inc.
- * $/LicenseInfo$
- */
-
-// Precompiled header
-#include "linden_common.h"
-// associated header
-#include "llcond.h"
-// STL headers
-// std headers
-// external library headers
-// other Linden headers
-
-namespace // anonymous
-{
-
-// See comments in LLCond::convert(const LLDate&) below
-std::time_t compute_lldate_epoch()
-{
-    LLDate lldate_epoch;
-    std::tm tm;
-    // It should be noted that calling LLDate::split() to write directly
-    // into a std::tm struct depends on S32 being a typedef for int in
-    // stdtypes.h: split() takes S32*, whereas tm fields are documented to
-    // be int. If you get compile errors here, somebody changed the
-    // definition of S32. You'll have to declare six S32 variables,
-    // split() into them, then assign them into the relevant tm fields.
-    if (! lldate_epoch.split(&tm.tm_year, &tm.tm_mon, &tm.tm_mday,
-                             &tm.tm_hour, &tm.tm_min, &tm.tm_sec))
-    {
-        // Theoretically split() could return false. In that case, we
-        // don't have valid data, so we can't compute offset, so skip the
-        // rest of this.
-        return 0;
-    }
-
-    tm.tm_isdst = 0;
-    std::time_t lldate_epoch_time = std::mktime(&tm);
-    if (lldate_epoch_time == -1)
-    {
-        // Theoretically mktime() could return -1, meaning that the contents
-        // of the passed std::tm cannot be represented as a time_t. (Worrisome
-        // if LLDate's epoch happened to be exactly 1 tick before
-        // std::time_t's epoch...)
-        // In the error case, assume offset 0.
-        return 0;
-    }
-
-    // But if we got this far, lldate_epoch_time is the time_t we want.
-    return lldate_epoch_time;
-}
-
-} // anonymous namespace
-
-// convert LLDate to a chrono::time_point
-std::chrono::system_clock::time_point LLCond::convert(const LLDate& lldate)
-{
-    // std::chrono::system_clock's epoch MAY be the Unix epoch, namely
-    // midnight UTC on 1970-01-01, in fact it probably is. But until C++20,
-    // system_clock does not guarantee that. Unfortunately time_t doesn't
-    // specify its epoch either, other than to note that it "almost always" is
-    // the Unix epoch (https://en.cppreference.com/w/cpp/chrono/c/time_t).
-    // LLDate, being based on apr_time_t, does guarantee 1970-01-01T00:00 UTC.
-    // http://apr.apache.org/docs/apr/1.5/group__apr__time.html#gadb4bde16055748190eae190c55aa02bb
-
-    // The easy, efficient conversion would be
-    // std::chrono::system_clock::from_time_t(std::time_t(LLDate::secondsSinceEpoch())).
-    // But that assumes that both time_t and system_clock have the same epoch
-    // as LLDate -- an assumption that will work until it unexpectedly doesn't.
-
-    // It would be more formally correct to break out the year, month, day,
-    // hour, minute, second (UTC) using LLDate::split() and recombine them
-    // into std::time_t using std::mktime(). However, both split() and
-    // mktime() have integer second granularity, whereas callers of
-    // wait_until() are very likely to be interested in sub-second precision.
-    // In that sense std::chrono::system_clock::from_time_t() is still
-    // preferred.
-
-    // So use the split() / mktime() mechanism to determine the numeric value
-    // of the LLDate / apr_time_t epoch as expressed in time_t. (We assume
-    // that the epoch offset can be expressed as integer seconds, per split()
-    // and mktime(), which seems plausible.)
-
-    // n.b. A function-static variable is initialized only once in a
-    // thread-safe way.
-    static std::time_t lldate_epoch_time = compute_lldate_epoch();
-
-    // LLDate::secondsSinceEpoch() gets us, of course, how long it has
-    // been since lldate_epoch_time. So adding lldate_epoch_time should
-    // give us the correct time_t representation of a given LLDate even if
-    // time_t's epoch differs from LLDate's.
-    // We don't have to worry about the relative epochs of time_t and
-    // system_clock because from_time_t() takes care of that!
-    return std::chrono::system_clock::from_time_t(lldate_epoch_time +
-                                                  lldate.secondsSinceEpoch());
-}
-
-// convert F32Milliseconds to a chrono::duration
-std::chrono::milliseconds LLCond::convert(F32Milliseconds)
-{
-    // extract the F32 milliseconds from F32Milliseconds, construct
-    // std::chrono::milliseconds from that value
-    return std::chrono::milliseconds(timeout_duration.value());
-}
diff --git a/indra/llcommon/llcond.h b/indra/llcommon/llcond.h
index 5ed9f10123..d18058cf62 100644
--- a/indra/llcommon/llcond.h
+++ b/indra/llcommon/llcond.h
@@ -15,7 +15,6 @@
 #define LL_LLCOND_H
 
 #include "llunits.h"
-#include "lldate.h"
 #include <boost/fiber/condition_variable.hpp>
 #include <mutex>
 #include <chrono>
@@ -134,44 +133,6 @@ public:
     }
 
     /**
-     * Pass wait_until() a chrono::time_point, indicating the time at which we
-     * should stop waiting, and a predicate accepting (const DATA&), returning
-     * bool. The predicate returns true when the condition for which it is
-     * waiting has been satisfied, presumably determined by examining the
-     * referenced DATA. wait_until() locks the mutex and, until the predicate
-     * returns true, calls wait_until() on the condition_variable.
-     * wait_until() returns false if condition_variable::wait_until() timed
-     * out without the predicate returning true.
-     */
-    template <typename Clock, typename Duration, typename Pred>
-    bool wait_until(const std::chrono::time_point<Clock, Duration>& timeout_time, Pred pred)
-    {
-        std::unique_lock<boost::fibers::mutex> lk(mMutex);
-        // see wait() for comments about this const_cast
-        while (! pred(const_cast<const value_type&>(mData)))
-        {
-            if (boost::fibers::cv_status::timeout == mCond.wait_until(lk, timeout_time))
-            {
-                // It's possible that wait_until() timed out AND the predicate
-                // became true more or less simultaneously. Even though
-                // wait_until() timed out, check the predicate one more time.
-                return pred(const_cast<const value_type&>(mData));
-            }
-        }
-        return true;
-    }
-
-    /**
-     * This wait_until() overload accepts LLDate as the time_point. Its
-     * semantics are the same as the generic wait_until() method.
-     */
-    template <typename Pred>
-    bool wait_until(const LLDate& timeout_time, Pred pred)
-    {
-        return wait_until(convert(timeout_time), pred);
-    }
-
-    /**
      * Pass wait_for() a chrono::duration, indicating how long we're willing
      * to wait, and a predicate accepting (const DATA&), returning bool. The
      * predicate returns true when the condition for which it is waiting has
@@ -207,10 +168,54 @@ public:
     }
 
 protected:
-    // convert LLDate to a chrono::time_point
-    std::chrono::system_clock::time_point convert(const LLDate&);
     // convert F32Milliseconds to a chrono::duration
-    std::chrono::milliseconds convert(F32Milliseconds);
+    std::chrono::milliseconds convert(F32Milliseconds)
+    {
+        // extract the F32 milliseconds from F32Milliseconds, construct
+        // std::chrono::milliseconds from that value
+        return { timeout_duration.value() };
+    }
+
+private:
+    /**
+     * Pass wait_until() a chrono::time_point, indicating the time at which we
+     * should stop waiting, and a predicate accepting (const DATA&), returning
+     * bool. The predicate returns true when the condition for which it is
+     * waiting has been satisfied, presumably determined by examining the
+     * referenced DATA. wait_until() locks the mutex and, until the predicate
+     * returns true, calls wait_until() on the condition_variable.
+     * wait_until() returns false if condition_variable::wait_until() timed
+     * out without the predicate returning true.
+     *
+     * Originally this class and its subclasses published wait_until() methods
+     * corresponding to each wait_for() method. But that raised all sorts of
+     * fascinating questions about the time zone of the passed time_point:
+     * local time? server time? UTC? The bottom line is that for LLCond
+     * timeout purposes, we really shouldn't have to care -- timeout duration
+     * is all we need. This private method remains because it's the simplest
+     * way to support iteratively waiting across spurious wakeups while
+     * honoring a fixed timeout.
+     */
+    template <typename Clock, typename Duration, typename Pred>
+    bool wait_until(const std::chrono::time_point<Clock, Duration>& timeout_time, Pred pred)
+    {
+        std::unique_lock<boost::fibers::mutex> lk(mMutex);
+        // We advise the caller to pass a predicate accepting (const DATA&).
+        // But what if they instead pass a predicate accepting non-const
+        // (DATA&)? Such a predicate could modify mData, which would be Bad.
+        // Forbid that.
+        while (! pred(const_cast<const value_type&>(mData)))
+        {
+            if (boost::fibers::cv_status::timeout == mCond.wait_until(lk, timeout_time))
+            {
+                // It's possible that wait_until() timed out AND the predicate
+                // became true more or less simultaneously. Even though
+                // wait_until() timed out, check the predicate one more time.
+                return pred(const_cast<const value_type&>(mData));
+            }
+        }
+        return true;
+    }
 };
 
 template <typename DATA>
@@ -222,7 +227,6 @@ public:
     using super::value_type;
     using super::get;
     using super::wait;
-    using super::wait_until;
     using super::wait_for;
 
     /// LLScalarCond can be explicitly initialized with a specific value for
@@ -258,31 +262,6 @@ public:
     }
 
     /**
-     * Pass wait_until_equal() a chrono::time_point, indicating the time at
-     * which we should stop waiting, and a value for which to wait.
-     * wait_until_equal() locks the mutex and, until the stored DATA equals
-     * that value, calls wait_until() on the condition_variable.
-     * wait_until_equal() returns false if condition_variable::wait_until()
-     * timed out without the stored DATA being equal to the passed value.
-     */
-    template <typename Clock, typename Duration>
-    bool wait_until_equal(const std::chrono::time_point<Clock, Duration>& timeout_time,
-                          const value_type& value)
-    {
-        return super::wait_until(timeout_time,
-                                 [&value](const value_type& data){ return (data == value); });
-    }
-
-    /**
-     * This wait_until_equal() overload accepts LLDate as the time_point. Its
-     * semantics are the same as the generic wait_until_equal() method.
-     */
-    bool wait_until_equal(const LLDate& timeout_time, const value_type& value)
-    {
-        return wait_until_equal(super::convert(timeout_time), value);
-    }
-
-    /**
      * Pass wait_for_equal() a chrono::duration, indicating how long we're
      * willing to wait, and a value for which to wait. wait_for_equal() locks
      * the mutex and, until the stored DATA equals that value, calls
@@ -320,31 +299,6 @@ public:
     }
 
     /**
-     * Pass wait_until_unequal() a chrono::time_point, indicating the time at
-     * which we should stop waiting, and a value from which to move away.
-     * wait_until_unequal() locks the mutex and, until the stored DATA no
-     * longer equals that value, calls wait_until() on the condition_variable.
-     * wait_until_unequal() returns false if condition_variable::wait_until()
-     * timed out with the stored DATA still being equal to the passed value.
-     */
-    template <typename Clock, typename Duration>
-    bool wait_until_unequal(const std::chrono::time_point<Clock, Duration>& timeout_time,
-                            const value_type& value)
-    {
-        return super::wait_until(timeout_time,
-                                 [&value](const value_type& data){ return (data != value); });
-    }
-
-    /**
-     * This wait_until_unequal() overload accepts LLDate as the time_point.
-     * Its semantics are the same as the generic wait_until_unequal() method.
-     */
-    bool wait_until_unequal(const LLDate& timeout_time, const value_type& value)
-    {
-        return wait_until_unequal(super::convert(timeout_time), value);
-    }
-
-    /**
      * Pass wait_for_unequal() a chrono::duration, indicating how long we're
      * willing to wait, and a value from which to move away.
      * wait_for_unequal() locks the mutex and, until the stored DATA no longer
@@ -387,13 +341,10 @@ public:
     using super::value_type;
     using super::get;
     using super::wait;
-    using super::wait_until;
     using super::wait_for;
     using super::wait_equal;
-    using super::wait_until_equal;
     using super::wait_for_equal;
     using super::wait_unequal;
-    using super::wait_until_unequal;
     using super::wait_for_unequal;
 
     /// The bool stored in LLOneShotCond is initially false
@@ -421,28 +372,6 @@ public:
     }
 
     /**
-     * Pass wait_until() a chrono::time_point, indicating the time at which we
-     * should stop waiting. wait_until() locks the mutex and, until the stored
-     * bool is true, calls wait_until() on the condition_variable.
-     * wait_until() returns false if condition_variable::wait_until() timed
-     * out without the stored bool being true.
-     */
-    template <typename Clock, typename Duration>
-    bool wait_until(const std::chrono::time_point<Clock, Duration>& timeout_time)
-    {
-        return super::wait_until_unequal(timeout_time, false);
-    }
-
-    /**
-     * This wait_until() overload accepts LLDate as the time_point.
-     * Its semantics are the same as the generic wait_until() method.
-     */
-    bool wait_until(const LLDate& timeout_time)
-    {
-        return wait_until(super::convert(timeout_time));
-    }
-
-    /**
      * Pass wait_for() a chrono::duration, indicating how long we're willing
      * to wait. wait_for() locks the mutex and, until the stored bool is true,
      * calls wait_for() on the condition_variable. wait_for() returns false if