From 9b7d4e274200c9bc07490208916f573a0202f7f8 Mon Sep 17 00:00:00 2001 From: Seth Hillbrand Date: Tue, 10 Dec 2019 05:49:28 -0800 Subject: [PATCH] Recleaning DLIST from pcbnew This was re-introduced by 5d3e6e3d444c87f17641057fe17f3c7c54a0b8fb The crash happened b/c we have to manage list containers in each element and minor adjustments cause the rest of the list to be lost. This commit re-implements it using std::iterators and deque Fixes #2623 | https://gitlab.com/kicad/code/kicad/issues/2623 --- include/class_board_item.h | 9 +-- pcbnew/board_connected_item.h | 11 ---- pcbnew/class_board_item.cpp | 10 ---- pcbnew/class_module.cpp | 4 +- pcbnew/dialogs/dialog_find.cpp | 106 +++++++++++++-------------------- pcbnew/dialogs/dialog_find.h | 34 +++++++---- qa/qa_utils/mocks.cpp | 7 +-- 7 files changed, 72 insertions(+), 109 deletions(-) diff --git a/include/class_board_item.h b/include/class_board_item.h index 4798ea7353..edec1303cc 100644 --- a/include/class_board_item.h +++ b/include/class_board_item.h @@ -212,16 +212,9 @@ public: // only MODULEs & TRACKs can be locked at this time. } - /** - * Function UnLink - * detaches this object from its owner. This base class implementation - * should work for all derived classes which are held in a DLIST<>. - */ - virtual void UnLink(); - /** * Function DeleteStructure - * deletes this object after UnLink()ing it from its owner if it has one. + * deletes this object after removing from its parent if it has one. */ void DeleteStructure(); diff --git a/pcbnew/board_connected_item.h b/pcbnew/board_connected_item.h index ab6dc3ae27..a3ffe2da67 100644 --- a/pcbnew/board_connected_item.h +++ b/pcbnew/board_connected_item.h @@ -230,17 +230,6 @@ public: { } - void UnLink() - { - /* if it were needed: - DHEAD* list = GetList(); - - wxASSERT( list ); - - list->remove( this ); - */ - } - //---------------------------------- /** diff --git a/pcbnew/class_board_item.cpp b/pcbnew/class_board_item.cpp index 67b41ae80d..ffedfff004 100644 --- a/pcbnew/class_board_item.cpp +++ b/pcbnew/class_board_item.cpp @@ -51,16 +51,6 @@ wxString BOARD_ITEM::ShowShape( STROKE_T aShape ) } -void BOARD_ITEM::UnLink() -{ - DLIST* list = (DLIST*) GetList(); - wxASSERT( list ); - - if( list ) - list->Remove( this ); -} - - BOARD* BOARD_ITEM::GetBoard() const { if( Type() == PCB_T ) diff --git a/pcbnew/class_module.cpp b/pcbnew/class_module.cpp index 94f1aa52f6..5d44695fdd 100644 --- a/pcbnew/class_module.cpp +++ b/pcbnew/class_module.cpp @@ -274,7 +274,7 @@ void MODULE::Add( BOARD_ITEM* aBoardItem, ADD_MODE aMode ) switch( aBoardItem->Type() ) { case PCB_MODULE_TEXT_T: - // Only user texts can be added this way. Reference and value are not hold in the DLIST. + // Only user text can be added this way. assert( static_cast( aBoardItem )->GetType() == TEXTE_MODULE::TEXT_is_DIVERS ); // no break @@ -321,7 +321,7 @@ void MODULE::Remove( BOARD_ITEM* aBoardItem ) switch( aBoardItem->Type() ) { case PCB_MODULE_TEXT_T: - // Only user texts can be removed this way. Reference and value are not hold in the DLIST. + // Only user text can be removed this way. wxCHECK_RET( static_cast( aBoardItem )->GetType() == TEXTE_MODULE::TEXT_is_DIVERS, "Please report this bug: Invalid remove operation on required text" ); diff --git a/pcbnew/dialogs/dialog_find.cpp b/pcbnew/dialogs/dialog_find.cpp index 404fe321a1..ae6b7b065d 100644 --- a/pcbnew/dialogs/dialog_find.cpp +++ b/pcbnew/dialogs/dialog_find.cpp @@ -51,12 +51,11 @@ bool FindIncludeTexts = true; bool FindIncludeValues = true; bool FindIncludeReferences = true; bool FindIncludeMarkers = true; -//bool findIncludeVias = false; + DIALOG_FIND::DIALOG_FIND( PCB_BASE_FRAME* aFrame ) : DIALOG_FIND_BASE( aFrame ) { m_frame = aFrame; - m_foundItem = NULL; GetSizer()->SetSizeHints( this ); m_searchCombo->Append( m_frame->GetFindHistoryList() ); @@ -84,10 +83,10 @@ DIALOG_FIND::DIALOG_FIND( PCB_BASE_FRAME* aFrame ) : DIALOG_FIND_BASE( aFrame ) m_includeMarkers->SetValue( FindIncludeMarkers ); m_status->SetLabel( wxEmptyString); - m_hitList = new DLIST; - m_hitList->SetOwnership( false ); - m_itemCount = 0; - isUpToDate = false; + m_upToDate = false; + + m_hitList.clear(); + m_it = m_hitList.begin(); m_findNext->SetDefault(); SetInitialFocus( m_searchCombo ); @@ -112,7 +111,7 @@ void DIALOG_FIND::onFindPreviousClick( wxCommandEvent& aEvent ) void DIALOG_FIND::onSearchAgainClick( wxCommandEvent& aEvent ) { - isUpToDate = false; + m_upToDate = false; search( true ); } @@ -132,7 +131,7 @@ void DIALOG_FIND::search( bool aDirection ) { m_searchCombo->Insert( searchString, 0 ); m_searchCombo->SetSelection( 0 ); - isUpToDate = false; + m_upToDate = false; m_frame->GetFindHistoryList().Insert( searchString, 0 ); if( m_searchCombo->GetCount() > 10 ) @@ -146,7 +145,7 @@ void DIALOG_FIND::search( bool aDirection ) m_searchCombo->Delete( index ); m_searchCombo->Insert( searchString, 0 ); m_searchCombo->SetSelection( 0 ); - isUpToDate = false; + m_upToDate = false; if( m_frame->GetFindHistoryList().Index( searchString ) ) m_frame->GetFindHistoryList().Remove( searchString ); @@ -160,19 +159,19 @@ void DIALOG_FIND::search( bool aDirection ) if( FindOptionCase != m_matchCase->GetValue() ) { FindOptionCase = m_matchCase->GetValue(); - isUpToDate = false; + m_upToDate = false; } if( FindOptionWords != m_matchWords->GetValue() ) { FindOptionWords = m_matchWords->GetValue(); - isUpToDate = false; + m_upToDate = false; } if( FindOptionWildcards != m_wildcards->GetValue() ) { FindOptionWildcards = m_wildcards->GetValue(); - isUpToDate = false; + m_upToDate = false; } FindOptionWrap = m_wrap->GetValue(); @@ -180,25 +179,25 @@ void DIALOG_FIND::search( bool aDirection ) if( FindIncludeTexts != m_includeTexts->GetValue() ) { FindIncludeTexts = m_includeTexts->GetValue(); - isUpToDate = false; + m_upToDate = false; } if( FindIncludeValues != m_includeValues->GetValue() ) { FindIncludeValues = m_includeValues->GetValue(); - isUpToDate = false; + m_upToDate = false; } if( FindIncludeReferences != m_includeReferences->GetValue() ) { FindIncludeReferences = m_includeReferences->GetValue(); - isUpToDate = false; + m_upToDate = false; } if( FindIncludeMarkers != m_includeMarkers->GetValue() ) { FindIncludeMarkers = m_includeMarkers->GetValue(); - isUpToDate = false; + m_upToDate = false; } if( FindOptionCase ) @@ -218,14 +217,10 @@ void DIALOG_FIND::search( bool aDirection ) m_frame->GetCanvas()->GetViewStart( &screen->m_StartVisu.x, &screen->m_StartVisu.y ); // Refresh the list of results - if( !isUpToDate ) + if( !m_upToDate ) { m_status->SetLabel( _( "Searching..." ) ); - - while( m_hitList->GetCount() > 0 ) - m_hitList->PopBack(); - - m_foundItem = NULL; + m_hitList.clear(); if( FindIncludeTexts || FindIncludeValues || FindIncludeReferences ) { @@ -236,7 +231,7 @@ void DIALOG_FIND::search( bool aDirection ) || ( module->Value().Matches( m_frame->GetFindReplaceData(), nullptr ) && FindIncludeValues ) ) { - m_hitList->Append( module ); + m_hitList.push_back( module ); } if( m_includeTexts->GetValue() ) @@ -248,7 +243,7 @@ void DIALOG_FIND::search( bool aDirection ) if( textItem && textItem->Matches( m_frame->GetFindReplaceData(), nullptr ) ) { - m_hitList->Append( module ); + m_hitList.push_back( module ); } } } @@ -262,7 +257,7 @@ void DIALOG_FIND::search( bool aDirection ) if( textItem && textItem->Matches( m_frame->GetFindReplaceData(), nullptr ) ) { - m_hitList->Append( textItem ); + m_hitList.push_back( textItem ); } } } @@ -275,39 +270,35 @@ void DIALOG_FIND::search( bool aDirection ) MARKER_PCB* marker = m_frame->GetBoard()->GetMARKER( i ); if( marker->Matches( m_frame->GetFindReplaceData(), nullptr ) ) - m_hitList->Append( marker ); + m_hitList.push_back( marker ); } } - m_itemCount = -1; + m_upToDate = true; + + if( aDirection ) + m_it = m_hitList.begin(); + else + m_it = m_hitList.end(); } // Do we want a sorting algorithm ? If so, implement it here. // Get the item to display - if( m_hitList->begin() == NULL ) + if( m_hitList.empty() ) { m_frame->SetStatusText( wxEmptyString ); - m_itemCount = 0; - m_foundItem = NULL; - } - else if( m_itemCount == -1 ) - { - m_foundItem = aDirection ? m_hitList->begin() : m_hitList->end(); - m_itemCount = aDirection ? 0 : (int) m_hitList->GetCount() - 1; - isUpToDate = true; } else { if( aDirection ) { - if( m_itemCount >= static_cast( m_hitList->GetCount() - 1 ) ) + m_it++; + + if( m_it == m_hitList.end() ) { if( m_wrap->GetValue() ) - { - m_itemCount = 0; - m_foundItem = m_hitList->begin(); - } + m_it = m_hitList.begin(); else { m_frame->SetStatusText( wxEmptyString ); @@ -315,20 +306,14 @@ void DIALOG_FIND::search( bool aDirection ) return; } } - else - { - m_itemCount++; - m_foundItem = dynamic_cast( m_foundItem->Next() ); - } } else { - if( m_itemCount <= 0 ) + if( m_it == m_hitList.begin() ) { if( m_wrap->GetValue() ) { - m_itemCount = (int) m_hitList->GetCount() - 1; - m_foundItem = m_hitList->end(); + m_it = m_hitList.end(); } else { @@ -337,24 +322,22 @@ void DIALOG_FIND::search( bool aDirection ) return; } } - else - { - m_itemCount--; - m_foundItem = dynamic_cast( m_foundItem->Back() ); - } + + m_it--; } } // Display the item - if( m_foundItem ) + if( m_it != m_hitList.end() ) { - m_frame->GetToolManager()->RunAction( PCB_ACTIONS::selectItem, true, m_foundItem ); - m_frame->FocusOnLocation( m_foundItem->GetPosition(), true ); + m_frame->GetToolManager()->RunAction( PCB_ACTIONS::selectItem, true, *m_it ); + m_frame->FocusOnLocation( ( *m_it )->GetPosition(), true ); msg.Printf( _( "\"%s\" found" ), searchString ); m_frame->SetStatusText( msg ); - msg.Printf( _( "Hit(s): %i / %i" ), m_itemCount + 1, m_hitList->GetCount() ); + msg.Printf( _( "Hit(s): %ld / %lu" ), std::distance( m_hitList.begin(), m_it ), + m_hitList.size() ); m_status->SetLabel( msg ); } else @@ -364,12 +347,11 @@ void DIALOG_FIND::search( bool aDirection ) msg.Printf( _( "\"%s\" not found" ), searchString ); DisplayError( this, msg, 10 ); - m_itemCount = 0; m_status->SetLabel( _( "No hits" ) ); } if( m_highlightCallback ) - m_highlightCallback( m_foundItem ); + m_highlightCallback( GetItem() ); } void DIALOG_FIND::onClose( wxCommandEvent& aEvent ) @@ -384,9 +366,5 @@ void DIALOG_FIND::onClose( wxCommandEvent& aEvent ) FindIncludeMarkers = m_includeMarkers->GetValue(); FindIncludeReferences = m_includeReferences->GetValue(); - while( m_hitList->GetCount() > 0 ) - m_hitList->PopBack(); - - delete m_hitList; EndModal( 1 ); } diff --git a/pcbnew/dialogs/dialog_find.h b/pcbnew/dialogs/dialog_find.h index a2d3f02aac..2904ce3475 100644 --- a/pcbnew/dialogs/dialog_find.h +++ b/pcbnew/dialogs/dialog_find.h @@ -3,7 +3,7 @@ * * Copyright (C) 2012 Marco Mattila * Copyright (C) 2006 Jean-Pierre Charras - * Copyright (C) 1992-2012 Kicad Developers, see AUTHORS.txt for contributors. + * Copyright (C) 1992-2019 Kicad Developers, see AUTHORS.txt for contributors. * * This program is free software; you can redistribute it and/or * modify it under the terms of the GNU General Public License @@ -27,10 +27,13 @@ #define DIALOG_FIND_BASE_H #include +#include +#include +#include + +#include + #include -#include -#include -#include using namespace std; @@ -39,11 +42,23 @@ class DIALOG_FIND : public DIALOG_FIND_BASE public: DIALOG_FIND( PCB_BASE_FRAME* aParent ); + /** + * Returns the currently found item or nullptr in the case of no items found + * @return + */ inline BOARD_ITEM* GetItem() const { - return m_foundItem; + if( m_it != m_hitList.end() ) + return *m_it; + else + return nullptr; } + /** + * Function to be called on each found event. Must be able to handle nullptr in the + * case where no item is found + * @param aCallback + */ void SetCallback( boost::function aCallback ) { m_highlightCallback = aCallback; @@ -51,11 +66,10 @@ public: private: - PCB_BASE_FRAME* m_frame; - int m_itemCount; - BOARD_ITEM* m_foundItem; - DLIST* m_hitList; - bool isUpToDate; + PCB_BASE_FRAME* m_frame; + std::deque m_hitList; + std::deque::iterator m_it; + bool m_upToDate; boost::function m_highlightCallback; diff --git a/qa/qa_utils/mocks.cpp b/qa/qa_utils/mocks.cpp index 2e8ced459b..60859cc2a1 100644 --- a/qa/qa_utils/mocks.cpp +++ b/qa/qa_utils/mocks.cpp @@ -150,10 +150,9 @@ DIALOG_FIND::DIALOG_FIND( PCB_BASE_FRAME* aParent ) : DIALOG_FIND_BASE( aParent { // these members are initialized to avoid warnings about non initialized vars m_frame = aParent; - m_itemCount = 0; - m_foundItem = nullptr; - m_hitList = nullptr; - isUpToDate = false; + m_hitList.clear(); + m_it = m_hitList.begin(); + m_upToDate = false; } void DIALOG_FIND::onFindNextClick( wxCommandEvent& aEvent )