Redo "Provide STL idiom for iterating tracks..." ...

... Being careful not to use operator == on a default-constructed
std::list::iterator, which violates assertions in the MSVC libraries.

This reverts commit 7fb5ec4b7ab4f9302bd94446db86924cd2b8d67f.

src/Menus.cpp

@@ -2909,13 +2909,16 @@ void AudacityProject::SortTracks(int flags)
 {
    size_t ndx = 0;
    int cmpValue;
+   // This one place outside of TrackList where we must use undisguised
+   // std::list iterators!  Avoid this elsewhere!
    std::vector<ListOfTracks::iterator> arr;
    arr.reserve(mTracks->GetCount());
    bool lastTrackLinked = false;
    //sort by linked tracks. Assumes linked track follows owner in list.
 
    // First find the permutation.
-   for (auto iter = mTracks->begin(), end = mTracks->end(); iter != end; ++iter) {
+   for (auto iter = mTracks->ListOfTracks::begin(),
+        end = mTracks->ListOfTracks::end(); iter != end; ++iter) {
       const auto &track = *iter;
       if(lastTrackLinked) {
          //insert after the last track since this track should be linked to it.
@@ -6242,7 +6245,7 @@ int AudacityProject::FindClips(double t0, double t1, bool next, std::vector<Foun
 
    bool anyWaveTracksSelected =
       tracks->end() != std::find_if(tracks->begin(), tracks->end(),
-         [] (const std::shared_ptr<Track>& t) {
+         [] (const Track * t) {
             return t->GetSelected() && t->GetKind() == Track::Wave; });
 
    // first search the tracks individually
@@ -7320,7 +7323,7 @@ int AudacityProject::FindClipBoundaries(double time, bool next, std::vector<Foun
 
    bool anyWaveTracksSelected =
       tracks->end() != std::find_if(tracks->begin(), tracks->end(),
-         [] (const std::shared_ptr<Track>& t) {
+         [] (const Track *t) {
             return t->GetSelected() && t->GetKind() == Track::Wave; });
 
 

src/Track.cpp

@@ -321,12 +321,18 @@ bool PlayableTrack::HandleXMLAttribute(const wxChar *attr, const wxChar *value)
 }
 
 // TrackListIterator
+TrackListIterator::TrackListIterator(TrackList * val, TrackNodePointer p)
+   : l{ val }
+   , cur{ p }
+{
+}
+
 TrackListIterator::TrackListIterator(TrackList * val)
-   : l(val)
+   : l{ val }
    , cur{}
 {
    if (l)
-      cur = l->begin();
+      cur = l->ListOfTracks::begin();
 }
 
 Track *TrackListIterator::StartWith(Track * val)
@@ -356,7 +362,7 @@ Track *TrackListIterator::First(TrackList * val)
       return NULL;
    }
 
-   cur = l->begin();
+   cur = l->ListOfTracks::begin();
 
    if (!l->isNull(cur)) {
       return cur->get();
@@ -371,7 +377,7 @@ Track *TrackListIterator::Last(bool skiplinked)
       return NULL;
    }
 
-   cur = l->end();
+   cur = l->ListOfTracks::end();
    if (l->hasPrev(cur))
       --cur;
    else
@@ -442,6 +448,14 @@ Track *TrackListIterator::Prev(bool skiplinked)
    return cur->get();
 }
 
+Track *TrackListIterator::operator *() const
+{
+   if ( !l || l->isNull( cur ) )
+      return nullptr;
+   else
+      return cur->get();
+}
+
 Track *TrackListIterator::RemoveCurrent()
 {
    if (!l || l->isNull(cur))
@@ -460,6 +474,21 @@ Track *TrackListIterator::RemoveCurrent()
    return NULL;
 }
 
+bool TrackListIterator::operator == (const TrackListIterator &other) const
+{
+   if (cur == other.cur)
+      // sufficient but not necessary
+      return true;
+
+   // Also return true whenever each contains either an end iterator or is
+   // in default-constructed state.
+   bool isEnd = !l || l->isNull( cur );
+   if (isEnd)
+      return !other.l || other.l->isNull( other.cur );
+
+   return false;
+}
+
 //
 // TrackListCondIterator (base class for iterators that iterate over all tracks
 // that meet a condition)
@@ -741,9 +770,11 @@ void TrackList::DoAssign(const TrackList &that)
 void TrackList::Swap(TrackList &that)
 {
    ListOfTracks::swap(that);
-   for (auto it = begin(), last = end(); it != last; ++it)
+   for (auto it = ListOfTracks::begin(), last = ListOfTracks::end();
+        it != last; ++it)
       (*it)->SetOwner(this->mSelf, it);
-   for (auto it = that.begin(), last = that.end(); it != last; ++it)
+   for (auto it = that.ListOfTracks::begin(), last = that.ListOfTracks::end();
+        it != last; ++it)
       (*it)->SetOwner(that.mSelf, it);
 }
 
@@ -770,8 +801,8 @@ void TrackList::RecalcPositions(TrackNodePointer node)
    }
 
    const auto theEnd = end();
-   for (auto n = node; n != theEnd; ++n) {
+   for (auto n = TrackListIterator{ this, node }; n != theEnd; ++n) {
-      t = n->get();
+      t = *n;
       t->SetIndex(i++);
       t->SetY(y);
       y += t->GetHeight();
@@ -803,12 +834,12 @@ void TrackList::ResizingEvent(TrackNodePointer node)
 void TrackList::Permute(const std::vector<TrackNodePointer> &permutation)
 {
    for (const auto iter : permutation) {
-      value_type track = std::move(*iter);
+      ListOfTracks::value_type track = std::move(*iter);
       erase(iter);
       Track *pTrack = track.get();
-      pTrack->SetOwner(mSelf, insert(end(), std::move(track)));
+      pTrack->SetOwner(mSelf, insert(ListOfTracks::end(), std::move(track)));
    }
-   auto n = begin();
+   auto n = ListOfTracks::begin();
    RecalcPositions(n);
    PermutationEvent();
 }
@@ -817,8 +848,8 @@ template<typename TrackKind>
 Track *TrackList::Add(std::unique_ptr<TrackKind> &&t)
 {
    Track *pTrack;
-   push_back(value_type(pTrack = t.release()));
+   push_back(ListOfTracks::value_type(pTrack = t.release()));
-   auto n = end();
+   auto n = ListOfTracks::end();
    --n;
    pTrack->SetOwner(mSelf, n);
    RecalcPositions(n);
@@ -839,8 +870,8 @@ template<typename TrackKind>
 Track *TrackList::AddToHead(std::unique_ptr<TrackKind> &&t)
 {
    Track *pTrack;
-   push_front(value_type(pTrack = t.release()));
+   push_front(ListOfTracks::value_type(pTrack = t.release()));
-   auto n = begin();
+   auto n = ListOfTracks::begin();
    pTrack->SetOwner(mSelf, n);
    RecalcPositions(n);
    ResizingEvent(n);
@@ -854,7 +885,7 @@ template<typename TrackKind>
 Track *TrackList::Add(std::shared_ptr<TrackKind> &&t)
 {
    push_back(t);
-   auto n = end();
+   auto n = ListOfTracks::end();
    --n;
    t->SetOwner(mSelf, n);
    RecalcPositions(n);
@@ -866,9 +897,10 @@ Track *TrackList::Add(std::shared_ptr<TrackKind> &&t)
 template Track *TrackList::Add<Track>(std::shared_ptr<Track> &&);
 template Track *TrackList::Add<WaveTrack>(std::shared_ptr<WaveTrack> &&);
 
-auto TrackList::Replace(Track * t, value_type &&with) -> value_type
+auto TrackList::Replace(Track * t, ListOfTracks::value_type &&with) ->
+   ListOfTracks::value_type
 {
-   value_type holder;
+   ListOfTracks::value_type holder;
    if (t && with) {
       auto node = t->GetNode();
       t->SetOwner({}, {});
@@ -888,13 +920,13 @@ auto TrackList::Replace(Track * t, value_type &&with) -> value_type
 
 TrackNodePointer TrackList::Remove(Track *t)
 {
-   TrackNodePointer result(end());
+   auto result = ListOfTracks::end();
    if (t) {
       auto node = t->GetNode();
       t->SetOwner({}, {});
 
       if (!isNull(node)) {
-         value_type holder = std::move( *node );
+         ListOfTracks::value_type holder = std::move( *node );
 
          result = erase(node);
          if (!isNull(result)) {
@@ -1052,7 +1084,7 @@ void TrackList::SwapNodes(TrackNodePointer s1, TrackNodePointer s2)
    }
 
    // Remove tracks
-   value_type save11 = std::move(*s1), save12{};
+   ListOfTracks::value_type save11 = std::move(*s1), save12{};
    s1 = erase(s1);
    if (linked1) {
       wxASSERT(s1 != s2);
@@ -1060,7 +1092,7 @@ void TrackList::SwapNodes(TrackNodePointer s1, TrackNodePointer s2)
    }
    const bool same = (s1 == s2);
 
-   value_type save21 = std::move(*s2), save22{};
+   ListOfTracks::value_type save21 = std::move(*s2), save22{};
    s2 = erase(s2);
    if (linked2)
       save22 = std::move(*s2), s2 = erase(s2);
@@ -1112,9 +1144,7 @@ bool TrackList::MoveDown(Track * t)
 
 bool TrackList::Contains(const Track * t) const
 {
-   return std::find_if(begin(), end(),
+   return make_iterator_range( *this ).contains( t );
-      [=](const value_type &track) { return t == track.get(); }
-   ) != end();
 }
 
 bool TrackList::IsEmpty() const
@@ -1136,12 +1166,12 @@ int TrackList::GetCount() const
 TimeTrack *TrackList::GetTimeTrack()
 {
    auto iter = std::find_if(begin(), end(),
-      [] (const value_type &t) { return t->GetKind() == Track::Time; }
+      [] ( Track *t ) { return t->GetKind() == Track::Time; }
    );
    if (iter == end())
       return nullptr;
    else
-      return static_cast<TimeTrack*>(iter->get());
+      return static_cast<TimeTrack*>(*iter);
 }
 
 const TimeTrack *TrackList::GetTimeTrack() const
@@ -1214,7 +1244,7 @@ unsigned TrackList::GetNumExportChannels(bool selectionOnly) const
 
 namespace {
    template<typename Array>
-   Array GetWaveTracks(ListOfTracks::const_iterator p, ListOfTracks::const_iterator end,
+   Array GetWaveTracks(TrackListIterator p, const TrackListIterator end,
                        bool selectionOnly, bool includeMuted)
    {
       Array waveTrackArray;
@@ -1225,8 +1255,7 @@ namespace {
          if (track->GetKind() == Track::Wave &&
             (includeMuted || !wt->GetMute()) &&
             (track->GetSelected() || !selectionOnly)) {
-            waveTrackArray.push_back(
+            waveTrackArray.push_back( Track::Pointer< WaveTrack >( track ) );
-               std::static_pointer_cast<WaveTrack>(track));
          }
       }
 
@@ -1241,7 +1270,9 @@ WaveTrackArray TrackList::GetWaveTrackArray(bool selectionOnly, bool includeMute
 
 WaveTrackConstArray TrackList::GetWaveTrackConstArray(bool selectionOnly, bool includeMuted) const
 {
-   return GetWaveTracks<WaveTrackConstArray>(begin(), end(), selectionOnly, includeMuted);
+   auto list = const_cast<TrackList*>(this);
+   return GetWaveTracks<WaveTrackConstArray>(
+      list->begin(), list->end(), selectionOnly, includeMuted);
 }
 
 #if defined(USE_MIDI)
@@ -1252,7 +1283,7 @@ NoteTrackArray TrackList::GetNoteTrackArray(bool selectionOnly)
    for(const auto &track : *this) {
       if (track->GetKind() == Track::Note &&
          (track->GetSelected() || !selectionOnly)) {
-         noteTrackArray.push_back(std::static_pointer_cast<NoteTrack>(track));
+         noteTrackArray.push_back( Track::Pointer<NoteTrack>(track) );
       }
    }
 

src/Track.h

@@ -337,13 +337,20 @@ protected:
 };
 
 class AUDACITY_DLL_API TrackListIterator /* not final */
+: public std::iterator< std::forward_iterator_tag, Track *const >
 {
  public:
-   TrackListIterator(TrackList * val = NULL);
+   // The default-constructed value can serve as the end iterator for
+   // traversal over any track list.
+   TrackListIterator() {}
+   explicit TrackListIterator(TrackList * val);
+   explicit TrackListIterator(TrackList * val, TrackNodePointer p);
+   TrackListIterator(const TrackListIterator&) = default;
+   TrackListIterator& operator=(const TrackListIterator&) = default;
    virtual ~TrackListIterator() {}
 
    // Iterate functions
-   virtual Track *First(TrackList * val = NULL);
+   virtual Track *First(TrackList * val = nullptr);
    virtual Track *StartWith(Track * val);
    virtual Track *Next(bool skiplinked = false);
    virtual Track *Prev(bool skiplinked = false);
@@ -351,19 +358,42 @@ class AUDACITY_DLL_API TrackListIterator /* not final */
 
    Track *RemoveCurrent(); // deletes track, returns next
 
+   // Provide minimal STL forward-iterator idiom:
+
+   // unlike Next, this is non-mutating.
+   // An end iterator may be safely dereferenced, returning nullptr.
+   Track *operator * () const;
+
+   TrackListIterator &operator++ () { (void) Next(); return *this; }
+
+   bool operator == (const TrackListIterator &other) const;
+   bool operator != (const TrackListIterator &other) const
+   { return !(*this == other); }
+
  protected:
    friend TrackList;
 
-   TrackList *l;
+   TrackList *l {};
    TrackNodePointer cur{};
 };
 
 class AUDACITY_DLL_API TrackListConstIterator
+: public std::iterator< std::forward_iterator_tag, const Track *const >
 {
 public:
-   TrackListConstIterator(const TrackList * val = NULL)
+   // The default-constructed value can serve as the end iterator for
+   // traversal over any track list.
+   TrackListConstIterator() {}
+   explicit TrackListConstIterator(
+      const TrackList * val, TrackNodePointer p)
+      : mIter(const_cast<TrackList*>(val), p)
+   {}
+   explicit TrackListConstIterator(
+      const TrackList * val)
       : mIter(const_cast<TrackList*>(val))
    {}
+   TrackListConstIterator(const TrackListConstIterator&) = default;
+   TrackListConstIterator& operator=(const TrackListConstIterator&) = default;
    ~TrackListConstIterator() {}
 
    // Iterate functions
@@ -378,6 +408,19 @@ public:
    const Track *Last(bool skiplinked = false)
    { return mIter.Last(skiplinked); }
 
+   // Provide minimal STL forward-iterator idiom:
+
+   // unlike Next, this is non-mutating.
+   // An end iterator may be safely dereferenced, returning nullptr.
+   const Track *operator * () const { return *mIter; }
+
+   TrackListConstIterator &operator++ () { (void) Next(); return *this; }
+
+   bool operator == (const TrackListConstIterator &other) const
+   { return mIter == other.mIter; }
+   bool operator != (const TrackListConstIterator &other) const
+   { return !(*this == other); }
+
 private:
    TrackListIterator mIter;
 };
@@ -532,6 +575,17 @@ class TrackList final : public wxEvtHandler, public ListOfTracks
    // Destructor
    virtual ~TrackList();
 
+   // Hide the inherited begin() and end()
+   using iterator = TrackListIterator;
+   using const_iterator = TrackListConstIterator;
+   using value_type = Track *;
+   iterator begin() { return iterator{ this, ListOfTracks::begin() }; }
+   iterator end() { return {}; }
+   const_iterator begin() const { return const_iterator{ this }; }
+   const_iterator end() const { return {}; }
+   const_iterator cbegin() const { return begin(); }
+   const_iterator cend() const { return end(); }
+
    friend class Track;
    friend class TrackListIterator;
    friend class SyncLockedTracksIterator;
@@ -549,7 +603,7 @@ class TrackList final : public wxEvtHandler, public ListOfTracks
    Track *Add(std::shared_ptr<TrackKind> &&t);
 
    /// Replace first track with second track, give back a holder
-   value_type Replace(Track * t, value_type &&with);
+   ListOfTracks::value_type Replace(Track * t, ListOfTracks::value_type &&with);
 
    /// Remove this Track or all children of this TrackList.
    /// Return an iterator to what followed the removed track.
@@ -629,11 +683,11 @@ class TrackList final : public wxEvtHandler, public ListOfTracks
 
 private:
    bool isNull(TrackNodePointer p) const
-   { return p == end(); }
+   { return p == ListOfTracks::end(); }
    void setNull(TrackNodePointer &p)
-   { p = end(); }
+   { p = ListOfTracks::end(); }
    bool hasPrev(TrackNodePointer p) const
-   { return p != begin(); }
+   { return p != ListOfTracks::begin(); }
 
    void DoAssign(const TrackList &that);
        

src/effects/Effect.cpp

@@ -2170,7 +2170,8 @@ void Effect::ReplaceProcessedTracks(const bool bGoodResult)
    // Assume resources need to be freed.
    wxASSERT(mOutputTracks); // Make sure we at least did the CopyInputTracks().
 
-   auto iterOut = mOutputTracks->begin(), iterEnd = mOutputTracks->end();
+   auto iterOut = mOutputTracks->ListOfTracks::begin(),
+      iterEnd = mOutputTracks->ListOfTracks::end();
 
    size_t cnt = mOMap.size();
    size_t i = 0;