From f60b35483dfdb218e25cc8973ef555885a034da0 Mon Sep 17 00:00:00 2001
From: Paul Davis <paul@linuxaudiosystems.com>
Date: Thu, 16 Jun 2022 22:11:56 -0600
Subject: [PATCH] temporal: fix major conceptual error managing Point reference
 to owner map

When TempoMap::copy_points() is called, the new points are intended to belong
to the (nascent) new map. But the copy constructor for the points leaves the
_map member of a Point unchanged, and so the new points reference the old
map (forever!). ::copy_points() must reset each Point to reference the new map.

Refactored the object that has the _map member, so that we could limit access
to its ::set_map() method to TempoMap.
---
 libs/temporal/tempo.cc         |  7 ++++---
 libs/temporal/temporal/tempo.h | 21 ++++++++++++++++-----
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/libs/temporal/tempo.cc b/libs/temporal/tempo.cc
index 650e41c0b0..507e84e593 100644
--- a/libs/temporal/tempo.cc
+++ b/libs/temporal/tempo.cc
@@ -58,7 +58,7 @@ Point::add_state (XMLNode & node) const
 }
 
 Point::Point (TempoMap const & map, XMLNode const & node)
-	: _map (&map)
+	: MapOwned (map)
 {
 	if (!node.get_property (X_("sclock"), _sclock)) {
 		throw failed_constructor();
@@ -781,8 +781,9 @@ TempoMap::copy_points (TempoMap const & other)
 
 	sort (p.begin(), p.end(), Point::ptr_sclock_comparator());
 
-	for (std::vector<Point*>::iterator pi = p.begin(); pi != p.end(); ++pi) {
-		_points.push_back (**pi);
+	for (auto & pi : p) {
+		pi->set_map (*this);
+		_points.push_back (*pi);
 	}
 }
 
diff --git a/libs/temporal/temporal/tempo.h b/libs/temporal/temporal/tempo.h
index ad8c6eb097..38cd9f504f 100644
--- a/libs/temporal/temporal/tempo.h
+++ b/libs/temporal/temporal/tempo.h
@@ -60,6 +60,20 @@ namespace Temporal {
 class Meter;
 class TempoMap;
 
+class MapOwned {
+ protected:
+	MapOwned (TempoMap const & map) : _map (&map) {}
+	~MapOwned () {}
+
+ public:
+	TempoMap const & map() const { return *_map; }
+
+ protected:
+	friend class TempoMap;
+	void set_map (TempoMap const & map) { _map = &map; }
+	TempoMap const * _map;
+};
+
 /* Conceptually, Point is similar to timepos_t. However, whereas timepos_t can
  * use the TempoMap to translate between time domains, Point cannot. Why not?
  * Because Point is foundational in building the tempo map, and we cannot
@@ -68,9 +82,9 @@ class TempoMap;
  */
 
 typedef boost::intrusive::list_base_hook<boost::intrusive::tag<struct point_tag>> point_hook;
-class /*LIBTEMPORAL_API*/ Point : public point_hook {
+class /*LIBTEMPORAL_API*/ Point : public point_hook, public MapOwned  {
   public:
-	LIBTEMPORAL_API Point (TempoMap const & map, superclock_t sc, Beats const & b, BBT_Time const & bbt) : _sclock (sc), _quarters (b), _bbt (bbt), _map (&map) {}
+	LIBTEMPORAL_API Point (TempoMap const & map, superclock_t sc, Beats const & b, BBT_Time const & bbt) : MapOwned (map), _sclock (sc), _quarters (b), _bbt (bbt)  {}
 	LIBTEMPORAL_API Point (TempoMap const & map, XMLNode const &);
 
 	LIBTEMPORAL_API virtual ~Point() {}
@@ -127,13 +141,10 @@ class /*LIBTEMPORAL_API*/ Point : public point_hook {
 	LIBTEMPORAL_API inline bool operator== (Point const & other) const { return _sclock == other._sclock; }
 	LIBTEMPORAL_API inline bool operator!= (Point const & other) const { return _sclock != other._sclock; }
 
-	LIBTEMPORAL_API TempoMap const & map() const { return *_map; }
-
   protected:
 	superclock_t     _sclock;
 	Beats            _quarters;
 	BBT_Time         _bbt;
-	TempoMap const * _map;
 
 	void add_state (XMLNode &) const;