From f06e2dd6d12a1973a06d5c092e28d93d98506fd4 Mon Sep 17 00:00:00 2001 From: Paul Davis Date: Tue, 11 Aug 2020 18:26:25 -0600 Subject: [PATCH] Temporal: remove constructors accepting scalar values from timepos_t/timecnt_t and force use of factory methods This allows us to differentiate between superclock_t and samplepos_t (and related types) which are all typedef'ed to the same underlying primitive C++ type. Without this, it would be impossible for the compiler or someone reading the code to know whether a scalar passed to a constructor for a timeline type is in units of samples or superclocks --- libs/temporal/tempo.cc | 6 +- libs/temporal/temporal/timeline.h | 120 ++++++++++++++--------------- libs/temporal/timeline.cc | 123 ++++++++++++------------------ 3 files changed, 111 insertions(+), 138 deletions(-) diff --git a/libs/temporal/tempo.cc b/libs/temporal/tempo.cc index 61371cadba..87ad7fc915 100644 --- a/libs/temporal/tempo.cc +++ b/libs/temporal/tempo.cc @@ -77,7 +77,7 @@ Point::time() const { switch (_map->time_domain()) { case AudioTime: - return timepos_t (sclock()); + return timepos_t::from_superclock (sclock()); case BeatTime: return timepos_t (beats()); case BarTime: @@ -2508,7 +2508,7 @@ TempoMap::full_duration_at (timepos_t const & pos, timecnt_t const & duration, T /* determine superclocks */ s = metric_at (p).superclock_at (p.beats()); /* return duration in sc */ - return timecnt_t (s - pos.superclocks(), pos); + return timecnt_t::from_superclock (s - pos.superclocks(), pos); break; } break; @@ -2545,7 +2545,7 @@ TempoMap::full_duration_at (timepos_t const & pos, timecnt_t const & duration, T abort (); /*NOTREACHED*/ - return timecnt_t (0, timepos_t()); + return timecnt_t::from_superclock (0); } diff --git a/libs/temporal/temporal/timeline.h b/libs/temporal/temporal/timeline.h index 3ff4457816..01d730bff7 100644 --- a/libs/temporal/temporal/timeline.h +++ b/libs/temporal/temporal/timeline.h @@ -38,18 +38,23 @@ namespace Temporal { class timecnt_t; -/* 62 bit time value. - * 63rd bit: indicates music or audio time value - * 64th bit: sign bit +/* 62 bit positional time value. Theoretically signed, but the intent is for to + * always be positive. If the flag bit is set (i.e. ::flagged() is true), the + * numerical value counts musical ticks; otherwise it counts superclocks. */ -class timepos_t : public int62_t { +class LIBTEMPORAL_API timepos_t : public int62_t { public: timepos_t () : int62_t (false, 0) {} - timepos_t (superclock_t s) : int62_t (false, s) {} explicit timepos_t (timecnt_t const &); /* will throw() if val is negative */ explicit timepos_t (Temporal::Beats const & b) : int62_t (false, b.to_ticks()) {} + /* superclock_t and samplepos_t are the same underlying primitive type, + * which means we cannot use polymorphism to differentiate them. + */ + static timepos_t from_superclock (superclock_t s) { return timepos_t (false, s); } + static timepos_t from_samples (superclock_t s) { return timepos_t (false, samples_to_superclock (s, _thread_sample_rate)); } + bool is_beats() const { return flagged(); } bool is_superclock() const { return !flagged(); } @@ -60,24 +65,6 @@ class timepos_t : public int62_t { int64_t ticks() const { if (is_beats()) return val(); return _ticks (); } Beats beats() const { if (is_beats()) return Beats::ticks (val()); return _beats (); } - /* return a timepos_t that is the next (later) possible position given - * this one - */ - - timepos_t increment () const { - return timepos_t (val() + 1); - } - - /* return a timepos_t that is the previous (earlier) possible position given - * this one - */ - timepos_t decrement () const { - if (is_beats()) { - return timepos_t (val() - 1); /* beats can go negative */ - } - return timepos_t (val() > 0 ? val() - 1 : val()); /* samples cannot go negative */ - } - timepos_t & operator= (timecnt_t const & t); /* will throw() if val is negative */ timepos_t & operator= (superclock_t s) { v = s; return *this; } timepos_t & operator= (Temporal::Beats const & b) { operator= (build (true, b.to_ticks())); return *this; } @@ -96,9 +83,15 @@ class timepos_t : public int62_t { bool operator>= (timepos_t const & other) const { if (is_beats() == other.is_beats()) return val() >= other.val(); return expensive_gte (other); } timepos_t operator+(timecnt_t const & d) const; - timepos_t operator+(timepos_t const & d) const { if (is_beats() == d.is_beats()) return timepos_t (v + d.ticks()); return expensive_add (d.superclocks()); } - timepos_t operator+(superclock_t s) const { if (is_superclock()) return timepos_t (v + s); return expensive_add (s); } - timepos_t operator+(Temporal::Beats const &b ) const { if (is_beats()) return timepos_t (ticks() + b.to_ticks()); return expensive_add (b); } + timepos_t operator+(timepos_t const & d) const { if (is_beats() == d.is_beats()) return timepos_t (is_beats(), val() + d.val()); return expensive_add (d); } + + timepos_t operator+(Temporal::Beats const &b ) const { if (is_beats()) return timepos_t (true, ticks() + b.to_ticks()); return expensive_add (b); } + + /* donn't provide operator+(samplepos_t) or operator+(superclock_t) + * because the compiler can't disambiguate them and neither can we. + * to add such types, use ::from_samples() or ::from_superclock() to + * create a timepo_t and then add that. + */ /* operator-() poses severe and thorny problems for a class that represents position on a timeline. * @@ -130,39 +123,45 @@ class timepos_t : public int62_t { */ /* computes the distance between this timepos_t and @param p - such that: this + distance = p + * such that: this + distance = p + * + * This means that if @param p is later than this, distance is positive; + * if @param p is earlier than this, distance is negative. - This means that if @param p is later than this, distance is positive; - if @param p is earlier than this, distance is negative. - - Note that the return value is a timecnt_t whose position member - is equal to the value of this. That means if the distance uses - musical time value, the distance may not have constant value - at other positions on the timeline. + * Note that the return value is a timecnt_t whose position member + * is equal to the value of this. That means if the distance uses + * musical time value, the distance may not have constant value + * at other positions on the timeline. */ timecnt_t distance (timecnt_t const & p) const; - timecnt_t distance (superclock_t s) const; timecnt_t distance (Temporal::Beats const & b) const; timecnt_t distance (timepos_t const & p) const; /* computes a new position value that is @param d earlier than this */ - timepos_t earlier (timepos_t const & d) const; /* treat d as distance measured from timeline origin */ timepos_t earlier (timecnt_t const & d) const; - timepos_t earlier (samplepos_t d) const; timepos_t earlier (Beats const & d) const; timepos_t earlier (BBT_Offset const & d) const; /* like ::earlier() but changes this. loosely equivalent to operator-= */ - timepos_t & shift_earlier (timecnt_t const & d); - timepos_t & shift_earlier (samplepos_t); timepos_t & shift_earlier (Temporal::Beats const &); timepos_t & shift_earlier (Temporal::BBT_Offset const &); + /* given the absence of operator- and thus also operator--, return a + * timepos_t that is the previous (earlier) possible position given + * this one + */ + timepos_t decrement () const { return timepos_t (false, val() > 0 ? val() - 1 : val()); /* cannot go negative */ } + + /* purely for reasons of symmetry with ::decrement(), return a + * timepos_t that is the next (later) possible position given this one + */ + timepos_t increment () const { return timepos_t (flagged(), val() + 1); } + timepos_t & operator+=(timecnt_t const & d); - timepos_t & operator+=(samplepos_t); + timepos_t & operator+=(timepos_t const & d); timepos_t & operator+=(Temporal::Beats const &); timepos_t & operator+=(Temporal::BBT_Offset const &); @@ -222,11 +221,10 @@ class timepos_t : public int62_t { /* used to compute distance when time domains do not match */ timecnt_t expensive_distance (timepos_t const & p) const; - timecnt_t expensive_distance (superclock_t s) const; - timecnt_t expensive_distance (Temporal::Beats const & b) const; + timecnt_t expensive_distance (Beats const &) const; timepos_t expensive_add (Temporal::Beats const &) const; - timepos_t expensive_add (superclock_t s) const; + timepos_t expensive_add (timepos_t const & s) const; using int62_t::operator int64_t; using int62_t::operator-; @@ -259,22 +257,29 @@ class timepos_t : public int62_t { class LIBTEMPORAL_API timecnt_t { public: /* default to zero superclocks @ zero */ - timecnt_t () : _distance (int62_t (false, 0)), _position (superclock_t(0)) {} + timecnt_t () : _distance (int62_t (false, 0)), _position (timepos_t::from_superclock (0)) {} + timecnt_t (timecnt_t const &other) : _distance (other.distance()), _position (other.position()) {} /* construct from timeline types */ - timecnt_t (timepos_t const & d, timepos_t const & p) : _distance (d), _position (p) { assert (p.is_beats() == d.is_beats()); } - timecnt_t (timecnt_t const &, timepos_t const & pos); + explicit timecnt_t (timepos_t const & d, timepos_t const & p) : _distance (d), _position (p) { assert (p.is_beats() == d.is_beats()); } + explicit timecnt_t (timecnt_t const &, timepos_t const & pos); /* construct from int62_t (which will be flagged or not) and timepos_t */ - timecnt_t (int62_t d, timepos_t p) : _distance (d), _position (p) { assert (p.is_beats() == d.flagged()); } + explicit timecnt_t (int62_t d, timepos_t p) : _distance (d), _position (p) { assert (p.is_beats() == d.flagged()); } - /* construct from position/distance primitives */ - explicit timecnt_t (superclock_t s, timepos_t const & pos) : _distance (false, s), _position (pos) { assert (_distance.flagged() == _position.is_beats()); } + /* construct from beats */ explicit timecnt_t (Temporal::Beats const & b, timepos_t const & pos) : _distance (true, b.to_ticks()), _position (pos) { assert ( _distance.flagged() == _position.is_beats()); } + /* superclock_t and samplepos_t are the same underlying primitive type, + * which means we cannot use polymorphism to differentiate them. + */ + static timecnt_t from_superclock (superclock_t s, timepos_t const & pos) { return timecnt_t (int62_t (false, s), pos); } + static timecnt_t from_samples (samplepos_t s, timepos_t const & pos) { return timecnt_t (int62_t (false, samples_to_superclock (s, _thread_sample_rate)), pos); } + /* Construct from just a distance value - position is assumed to be zero */ - explicit timecnt_t (superclock_t s) : _distance (false, s), _position (superclock_t (0)) {} explicit timecnt_t (Temporal::Beats const & b) : _distance (true, b.to_ticks()), _position (Beats()) {} + static timecnt_t from_superclock (superclock_t s) { return timecnt_t (int62_t (false, s), timepos_t::from_superclock (0)); } + static timecnt_t from_samples (samplepos_t s) { return timecnt_t (int62_t (false, samples_to_superclock (s, _thread_sample_rate)), timepos_t::from_superclock (0)); } int62_t const & distance() const { return _distance; } timepos_t const & position() const { return _position; } @@ -385,7 +390,7 @@ namespace std { /* The utility of these two specializations of std::numeric_limits<> is not * clear, since both timepos_t and timecnt_t have a time domain, and comparing - * across time domains, while possible, is expensive. + * across time domains, while possible, is expensive. * * To avoid hidden costs, it seems easier to use timepos_t::max and * timecnt_t::max although these have a similar fundamental problem. @@ -394,21 +399,17 @@ namespace std { template<> struct numeric_limits { static Temporal::timepos_t min() { - return Temporal::timepos_t (0); + return Temporal::timepos_t::from_superclock (0); } static Temporal::timepos_t max() { - return Temporal::timepos_t (4611686018427387904); /* pow (2,62) */ + return Temporal::timepos_t::from_superclock (4611686018427387904); /* pow (2,62) */ } }; template<> struct numeric_limits { - static Temporal::timecnt_t min() { - return Temporal::timecnt_t (Temporal::superclock_t (0), Temporal::timepos_t (Temporal::superclock_t (0))); - } - static Temporal::timecnt_t max() { - return Temporal::timecnt_t (Temporal::superclock_t (4611686018427387904), Temporal::timepos_t (Temporal::superclock_t (0))); /* pow (2,62) */ - } + static Temporal::timecnt_t min() { return Temporal::timecnt_t::from_superclock (0); } + static Temporal::timecnt_t max() { return Temporal::timecnt_t::from_superclock (4611686018427387904); /* pow (2,62) */ } }; } @@ -454,4 +455,3 @@ inline static bool operator>= (Temporal::Beats const & b, Temporal::timecnt_t co #undef TEMPORAL_DOMAIN_WARNING #endif /* __libtemporal_timeline_h__ */ - diff --git a/libs/temporal/timeline.cc b/libs/temporal/timeline.cc index fabbf8ec46..be4746bff6 100644 --- a/libs/temporal/timeline.cc +++ b/libs/temporal/timeline.cc @@ -67,7 +67,7 @@ static TemporalStatistics stats; /* timecnt */ -timecnt_t timecnt_t::_max_timecnt (int62_t::max - 1, timepos_t()); +timecnt_t timecnt_t::_max_timecnt (timecnt_t::from_superclock (int62_t::max - 1)); timecnt_t::timecnt_t (timecnt_t const & tc, timepos_t const & pos) : _position (pos) @@ -112,13 +112,17 @@ timecnt_t::compute_beats() const timecnt_t timecnt_t::operator*(ratio_t const & r) const { - return timecnt_t (int_div_round (_distance.val() * r.numerator(), r.denominator()), _position); + const int62_t v (_distance.flagged(), int_div_round (_distance.val() * r.numerator(), r.denominator())); + return timecnt_t (v, _position); } timecnt_t timecnt_t::operator/(ratio_t const & r) const { - return timecnt_t (int_div_round (_distance.val() * r.denominator(), r.numerator()), _position); + /* note: x / (N/D) => x * (D/N) => (x * D) / N */ + + const int62_t v (_distance.flagged(), int_div_round (_distance.val() * r.denominator(), r.numerator())); + return timecnt_t (v, _position); } timecnt_t @@ -260,14 +264,21 @@ timepos_t::_ticks () const timepos_t timepos_t::expensive_add (Beats const & b) const { - assert (is_beats()); + assert (!is_beats()); + return timepos_t (beats() + b); } timepos_t -timepos_t::expensive_add (superclock_t sc) const +timepos_t::expensive_add (timepos_t const & t) const { - return timepos_t (val() + sc); + assert (is_beats() != t.is_beats ()); + + if (is_beats()) { + return timepos_t (beats() + t.beats()); + } + + return timepos_t::from_superclock (superclocks() + t.superclocks()); } /* */ @@ -276,21 +287,11 @@ timepos_t::expensive_add (superclock_t sc) const * thus returns a positive value if this condition is satisfied. */ -timecnt_t -timepos_t::distance (superclock_t s) const -{ - if (is_superclock()) { - return timecnt_t (s - val(), *this); - } - - return expensive_distance (s); -} - timecnt_t timepos_t::distance (Beats const & b) const { if (is_beats()) { - return timecnt_t ((b - _beats()).to_ticks(), *this); + return timecnt_t (b - _beats(), *this); } return expensive_distance (b); @@ -303,17 +304,7 @@ timepos_t::distance (timepos_t const & d) const return distance (d._beats()); } - return distance (d._superclocks()); -} - -timecnt_t -timepos_t::expensive_distance (superclock_t s) const -{ - if (is_superclock() && (val() < s)) { - PBD::warning << string_compose (_("programming warning: sample arithmetic will generate negative sample time (%1 - %2)"), superclocks(), s) << endmsg; - } - - return timecnt_t (s - superclocks(), *this); + return expensive_distance (d); } timecnt_t @@ -322,27 +313,18 @@ timepos_t::expensive_distance (Temporal::Beats const & b) const return timecnt_t (b - beats(), *this); } -/* */ -timepos_t -timepos_t:: earlier (superclock_t s) const +timecnt_t +timepos_t::expensive_distance (timepos_t const & p) const { - superclock_t sc; - if (is_beats()) { - TempoMap::SharedPtr tm (TempoMap::use()); - sc = tm->superclock_at (beats()); - } else { - sc = val(); + return timecnt_t (beats() + p.beats(), *this); } - - if (sc < s) { - PBD::warning << string_compose (_("programming warning: sample arithmetic will generate negative sample time (%1 - %2)"), superclocks(), s) << endmsg; - } - - return timepos_t (sc - s); + return timecnt_t::from_superclock (superclocks() + p.superclocks(), *this); } +/* */ + timepos_t timepos_t::earlier (Temporal::Beats const & b) const { @@ -389,23 +371,6 @@ timepos_t::shift_earlier (timecnt_t const & d) return shift_earlier (d.beats()); } -timepos_t & -timepos_t:: shift_earlier (superclock_t s) -{ - superclock_t sc; - - if (is_beats ()) { - TempoMap::SharedPtr tm (TempoMap::use()); - sc = tm->superclock_at (beats()); - } else { - sc = val(); - } - - v = sc - s; - - return *this; -} - timepos_t & timepos_t::shift_earlier (Temporal::Beats const & b) { @@ -425,19 +390,6 @@ timepos_t::shift_earlier (Temporal::Beats const & b) /* */ -timepos_t & -timepos_t:: operator+= (superclock_t s) -{ - if (is_superclock()) { - v += s; - } else { - TempoMap::SharedPtr tm (TempoMap::use()); - v = build (true, tm->scwalk_to_quarters (beats(), s).to_ticks()); - } - - return *this; -} - timepos_t & timepos_t::operator+=(Temporal::Beats const & b) { @@ -457,7 +409,7 @@ timepos_t timepos_t::operator+(timecnt_t const & d) const { if (d.time_domain() == AudioTime) { - return operator+ (d.superclocks()); + return operator+ (timepos_t::from_superclock (d.superclocks())); } return operator+ (d.beats()); @@ -467,11 +419,32 @@ timepos_t & timepos_t::operator+=(timecnt_t const & d) { if (d.time_domain() == AudioTime) { - return operator+= (d.superclocks()); + return operator+= (timepos_t::from_superclock (d.superclocks())); } return operator+= (d.beats()); } +/* */ + +timepos_t & +timepos_t::operator+=(timepos_t const & d) +{ + if (d.is_beats() == is_beats()) { + + v = build (flagged(), val() + d.val()); + + } else { + + if (is_beats()) { + v = build (false, val() + d.ticks()); + } else { + v = build (true, val() + d.superclocks()); + } + } + + return *this; +} + std::ostream& std::operator<< (std::ostream & o, timepos_t const & tp)