From da8be848e1297d07a6d300deca8a2dae229cf8e2 Mon Sep 17 00:00:00 2001 From: Jeff Young Date: Tue, 21 Oct 2025 12:58:39 +0100 Subject: [PATCH] Multiple bug fixes in handling of inheritance chains. Lots of code assumed a single inheritance, while in fact the parent could also be derived. Also removed a couple of cover types on std:shared_ptr and std::weak_ptr as it's important to know the API contract you're dealing with. Fixes https://gitlab.com/kicad/code/kicad/-/issues/22003 (cherry picked from commit 857c408b253eed202a136dff95c23cfb7367424b) --- .../dialogs/dialog_lib_symbol_properties.cpp | 4 +- .../dialogs/dialog_update_symbol_fields.cpp | 3 +- eeschema/eeschema_jobs_handler.cpp | 4 +- eeschema/fields_grid_table.h | 1 - eeschema/lib_symbol.cpp | 93 +++++++------------ eeschema/lib_symbol.h | 21 ++--- eeschema/project_rescue.cpp | 2 +- eeschema/symbol_editor/symbol_editor.cpp | 25 +++-- eeschema/symbol_library_manager.cpp | 4 +- eeschema/symbol_viewer_frame.cpp | 20 +--- qa/tests/eeschema/test_lib_part.cpp | 4 +- 11 files changed, 69 insertions(+), 112 deletions(-) diff --git a/eeschema/dialogs/dialog_lib_symbol_properties.cpp b/eeschema/dialogs/dialog_lib_symbol_properties.cpp index 410e624e3e..4abfc41092 100644 --- a/eeschema/dialogs/dialog_lib_symbol_properties.cpp +++ b/eeschema/dialogs/dialog_lib_symbol_properties.cpp @@ -197,7 +197,7 @@ bool DIALOG_LIB_SYMBOL_PROPERTIES::TransferDataToWindow() // and mark any matching names as inherited rather than duplicating them. if( m_libEntry->IsAlias() ) { - if( LIB_SYMBOL_SPTR parent = m_libEntry->GetParent().lock() ) + if( std::shared_ptr parent = m_libEntry->GetParent().lock() ) { std::vector parentFields; parent->GetFields( parentFields ); @@ -330,7 +330,7 @@ bool DIALOG_LIB_SYMBOL_PROPERTIES::TransferDataToWindow() m_inheritanceSelectCombo->Append( symbolNames ); - if( LIB_SYMBOL_SPTR rootSymbol = m_libEntry->GetParent().lock() ) + if( std::shared_ptr rootSymbol = m_libEntry->GetParent().lock() ) { wxString parentName = UnescapeString( rootSymbol->GetName() ); int selection = m_inheritanceSelectCombo->FindString( parentName ); diff --git a/eeschema/dialogs/dialog_update_symbol_fields.cpp b/eeschema/dialogs/dialog_update_symbol_fields.cpp index e4c1f3495c..da4479a13e 100644 --- a/eeschema/dialogs/dialog_update_symbol_fields.cpp +++ b/eeschema/dialogs/dialog_update_symbol_fields.cpp @@ -44,7 +44,8 @@ DIALOG_UPDATE_SYMBOL_FIELDS::DIALOG_UPDATE_SYMBOL_FIELDS( SYMBOL_EDIT_FRAME* aPa wxASSERT( aParent ); wxASSERT( aSymbol ); - m_parentSymbolReadOnly->SetValue( UnescapeString( m_symbol->GetParent().lock()->GetName() ) ); + if( std::shared_ptr parent = m_symbol->GetParent().lock() ) + m_parentSymbolReadOnly->SetValue( UnescapeString( parent->GetName() ) ); for( int i = 0; i < MANDATORY_FIELD_COUNT; ++i ) { diff --git a/eeschema/eeschema_jobs_handler.cpp b/eeschema/eeschema_jobs_handler.cpp index 83f00c9794..61fac151c2 100644 --- a/eeschema/eeschema_jobs_handler.cpp +++ b/eeschema/eeschema_jobs_handler.cpp @@ -858,8 +858,8 @@ int EESCHEMA_JOBS_HANDLER::doSymExportSvg( JOB_SYM_EXPORT_SVG* aSvgJob, { wxCHECK( symbol, CLI::EXIT_CODES::ERR_UNKNOWN ); - LIB_SYMBOL_SPTR parent; - LIB_SYMBOL* symbolToPlot = symbol; + std::shared_ptr parent; + LIB_SYMBOL* symbolToPlot = symbol; // if the symbol is an alias, then the draw items are stored in the root symbol if( symbol->IsAlias() ) diff --git a/eeschema/fields_grid_table.h b/eeschema/fields_grid_table.h index 42a321d887..5b594f7390 100644 --- a/eeschema/fields_grid_table.h +++ b/eeschema/fields_grid_table.h @@ -134,7 +134,6 @@ public: m_isInherited[aRow] = true; } - bool IsInherited( size_t aRow ) const { if( aRow >= m_isInherited.size() || aRow >= m_parentFields.size() ) diff --git a/eeschema/lib_symbol.cpp b/eeschema/lib_symbol.cpp index cb89ee32a0..8b83ea77d2 100644 --- a/eeschema/lib_symbol.cpp +++ b/eeschema/lib_symbol.cpp @@ -126,10 +126,7 @@ LIB_SYMBOL::LIB_SYMBOL( const wxString& aName, LIB_SYMBOL* aParent, SYMBOL_LIB* GetDescriptionField().SetVisible( false ); SetName( aName ); - - if( aParent ) - SetParent( aParent ); - + SetParent( aParent ); SetLib( aLibrary ); } @@ -172,10 +169,7 @@ LIB_SYMBOL::LIB_SYMBOL( const LIB_SYMBOL& aSymbol, SYMBOL_LIB* aLibrary ) : } } - LIB_SYMBOL_SPTR parent = aSymbol.m_parent.lock(); - - if( parent ) - SetParent( parent.get() ); + SetParent( aSymbol.m_parent.lock().get() ); } @@ -213,10 +207,7 @@ const LIB_SYMBOL& LIB_SYMBOL::operator=( const LIB_SYMBOL& aSymbol ) m_drawings.sort(); - LIB_SYMBOL_SPTR parent = aSymbol.m_parent.lock(); - - if( parent ) - SetParent( parent.get() ); + SetParent( aSymbol.m_parent.lock().get() ); EMBEDDED_FILES::operator=( aSymbol ); @@ -255,27 +246,17 @@ LIB_SYMBOL* LIB_SYMBOL::GetDummy() unsigned LIB_SYMBOL::GetInheritanceDepth() const { - unsigned depth = 0; + if( const std::shared_ptr parent = GetParent().lock() ) + return parent->GetInheritanceDepth() + 1; - LIB_SYMBOL_SPTR parent = GetParent().lock(); - - while( parent ) - { - depth += 1; - parent = parent->GetParent().lock(); - } - - return depth; + return 0; } -LIB_SYMBOL_SPTR LIB_SYMBOL::GetRootSymbol() const +std::shared_ptr LIB_SYMBOL::GetRootSymbol() const { - const LIB_SYMBOL_SPTR sp = m_parent.lock(); - - // Recurse until the parent symbol is empty. - if( sp ) - return sp->GetRootSymbol(); + if( const std::shared_ptr parent = m_parent.lock() ) + return parent->GetRootSymbol(); return m_me; } @@ -350,10 +331,9 @@ std::unique_ptr< LIB_SYMBOL > LIB_SYMBOL::Flatten() const if( IsAlias() ) { - LIB_SYMBOL_SPTR parent = m_parent.lock(); + std::shared_ptr parent = m_parent.lock(); - wxCHECK_MSG( parent, retv, - wxString::Format( "Parent of derived symbol '%s' undefined", m_name ) ); + wxCHECK_MSG( parent, retv, wxString::Format( "Parent of derived symbol '%s' undefined", m_name ) ); // Copy the parent. if( parent->IsAlias() ) @@ -447,7 +427,7 @@ bool LIB_SYMBOL::IsPower() const void LIB_SYMBOL::SetPower() { - if( LIB_SYMBOL_SPTR parent = m_parent.lock() ) + if( std::shared_ptr parent = m_parent.lock() ) { if( parent->IsRoot() ) parent->m_options = ENTRY_POWER; @@ -461,13 +441,8 @@ void LIB_SYMBOL::SetPower() bool LIB_SYMBOL::IsNormal() const { - if( LIB_SYMBOL_SPTR parent = m_parent.lock() ) - { - if( parent->IsRoot() ) - return parent->m_options == ENTRY_NORMAL; - else - return parent->IsNormal(); - } + if( const std::shared_ptr parent = m_parent.lock() ) + return parent->IsNormal(); return m_options == ENTRY_NORMAL; } @@ -475,12 +450,10 @@ bool LIB_SYMBOL::IsNormal() const void LIB_SYMBOL::SetNormal() { - if( LIB_SYMBOL_SPTR parent = m_parent.lock() ) + if( std::shared_ptr parent = m_parent.lock() ) { - if( parent->IsRoot() ) - parent->m_options = ENTRY_NORMAL; - else - parent->SetNormal(); + parent->SetNormal(); + return; } m_options = ENTRY_NORMAL; @@ -865,6 +838,9 @@ void LIB_SYMBOL::AddDrawItem( SCH_ITEM* aItem, bool aSort ) std::vector LIB_SYMBOL::GetPins( int aUnit, int aBodyStyle ) const { + if( std::shared_ptr parent = m_parent.lock() ) + return parent->GetPins( aUnit, aBodyStyle ); + std::vector pins; /* Notes: @@ -874,10 +850,7 @@ std::vector LIB_SYMBOL::GetPins( int aUnit, int aBodyStyle ) const * when m_bodyStyle == 0, the item is common to all body styles */ - LIB_SYMBOL_SPTR parent = m_parent.lock(); - const LIB_ITEMS_CONTAINER& drawItems = parent ? parent->m_drawings : m_drawings; - - for( const SCH_ITEM& item : drawItems[SCH_PIN_T] ) + for( const SCH_ITEM& item : m_drawings[SCH_PIN_T] ) { // Unit filtering: if( aUnit && item.m_unit && ( item.m_unit != aUnit ) ) @@ -986,6 +959,9 @@ const BOX2I LIB_SYMBOL::GetUnitBoundingBox( int aUnit, int aBodyStyle, { BOX2I bBox; // Start with a fresh BOX2I so the Merge algorithm works + if( std::shared_ptr parent = m_parent.lock() ) + bBox = parent->GetUnitBoundingBox( aUnit, aBodyStyle, aIgnoreHiddenFields ); + for( const SCH_ITEM& item : m_drawings ) { if( item.m_unit > 0 && m_unitCount > 1 && aUnit > 0 && aUnit != item.m_unit ) @@ -1312,14 +1288,8 @@ bool LIB_SYMBOL::HasAlternateBodyStyle() const return true; } - if( LIB_SYMBOL_SPTR parent = m_parent.lock() ) - { - for( const SCH_ITEM& item : parent->GetDrawItems() ) - { - if( item.m_bodyStyle > BODY_STYLE::BASE ) - return true; - } - } + if( std::shared_ptr parent = m_parent.lock() ) + return parent->HasAlternateBodyStyle(); return false; } @@ -1327,11 +1297,12 @@ bool LIB_SYMBOL::HasAlternateBodyStyle() const int LIB_SYMBOL::GetMaxPinNumber() const { - int maxPinNumber = 0; - LIB_SYMBOL_SPTR parent = m_parent.lock(); - const LIB_ITEMS_CONTAINER& drawItems = parent ? parent->m_drawings : m_drawings; + if( std::shared_ptr parent = m_parent.lock() ) + return parent->GetMaxPinNumber(); - for( const SCH_ITEM& item : drawItems[SCH_PIN_T] ) + int maxPinNumber = 0; + + for( const SCH_ITEM& item : m_drawings[SCH_PIN_T] ) { const SCH_PIN* pin = static_cast( &item ); long currentPinNumber = 0; @@ -1473,7 +1444,7 @@ void LIB_SYMBOL::SetUnitCount( int aCount, bool aDuplicateDrawItems ) int LIB_SYMBOL::GetUnitCount() const { - if( LIB_SYMBOL_SPTR parent = m_parent.lock() ) + if( std::shared_ptr parent = m_parent.lock() ) return parent->GetUnitCount(); return m_unitCount; diff --git a/eeschema/lib_symbol.h b/eeschema/lib_symbol.h index f454cc142b..797e6528a8 100644 --- a/eeschema/lib_symbol.h +++ b/eeschema/lib_symbol.h @@ -49,8 +49,6 @@ namespace KIFONT } -typedef std::shared_ptr LIB_SYMBOL_SPTR; ///< shared pointer to LIB_SYMBOL -typedef std::weak_ptr LIB_SYMBOL_REF; ///< weak pointer to LIB_SYMBOL typedef MULTIVECTOR LIB_ITEMS_CONTAINER; typedef LIB_ITEMS_CONTAINER::ITEM_PTR_VECTOR LIB_ITEMS; @@ -92,7 +90,7 @@ public: {} /// http://www.boost.org/doc/libs/1_55_0/libs/smart_ptr/sp_techniques.html#weak_without_shared. - LIB_SYMBOL_SPTR SharedPtr() const { return m_me; } + std::shared_ptr SharedPtr() const { return m_me; } /** * Create a copy of a LIB_SYMBOL and assigns unique KIIDs to the copy and its children. @@ -114,8 +112,8 @@ public: static LIB_SYMBOL* GetDummy(); void SetParent( LIB_SYMBOL* aParent = nullptr ); - LIB_SYMBOL_REF& GetParent() { return m_parent; } - const LIB_SYMBOL_REF& GetParent() const { return m_parent; } + std::weak_ptr& GetParent() { return m_parent; } + const std::weak_ptr& GetParent() const { return m_parent; } /** * Get the number of parents for this symbol. @@ -132,7 +130,7 @@ public: * * @return the weak_ptr to the root symbol of this symbol. */ - LIB_SYMBOL_SPTR GetRootSymbol() const; + std::shared_ptr GetRootSymbol() const; virtual wxString GetClass() const override { @@ -170,7 +168,7 @@ public: { if( GetDescriptionField().GetText().IsEmpty() && IsAlias() ) { - if( LIB_SYMBOL_SPTR parent = m_parent.lock() ) + if( std::shared_ptr parent = m_parent.lock() ) return parent->GetDescription(); } @@ -183,7 +181,7 @@ public: { if( m_keyWords.IsEmpty() && IsAlias() ) { - if( LIB_SYMBOL_SPTR parent = m_parent.lock() ) + if( std::shared_ptr parent = m_parent.lock() ) return parent->GetKeyWords(); } @@ -218,7 +216,7 @@ public: { if( m_fpFilters.IsEmpty() && IsAlias() ) { - if( LIB_SYMBOL_SPTR parent = m_parent.lock() ) + if( std::shared_ptr parent = m_parent.lock() ) return parent->GetFPFilters(); } @@ -671,8 +669,9 @@ private: void deleteAllFields(); private: - LIB_SYMBOL_SPTR m_me; - LIB_SYMBOL_REF m_parent; ///< Use for inherited symbols. + std::shared_ptr m_me; + std::weak_ptr m_parent; ///< Use for inherited symbols. + LIB_ID m_libId; LIB_ID m_sourceLibId; ///< For database library symbols; the original symbol timestamp_t m_lastModDate; diff --git a/eeschema/project_rescue.cpp b/eeschema/project_rescue.cpp index f1516b5c8f..43507287e2 100644 --- a/eeschema/project_rescue.cpp +++ b/eeschema/project_rescue.cpp @@ -426,7 +426,7 @@ void RESCUE_SYMBOL_LIB_TABLE_CANDIDATE::FindRescues( RESCUER& aRescuer, if( !cache_match && !lib_match ) continue; - LIB_SYMBOL_SPTR lib_match_parent; + std::shared_ptr lib_match_parent; // If it's a derive symbol, use the parent symbol to perform the pin test. if( lib_match && lib_match->IsAlias() ) diff --git a/eeschema/symbol_editor/symbol_editor.cpp b/eeschema/symbol_editor/symbol_editor.cpp index 28948d384f..ccae65688e 100644 --- a/eeschema/symbol_editor/symbol_editor.cpp +++ b/eeschema/symbol_editor/symbol_editor.cpp @@ -541,13 +541,13 @@ void SYMBOL_EDIT_FRAME::SaveSymbolCopyAs( bool aOpenCopy ) * * If the symbol is not an alias, the list will contain only the symbol itself. */ -static std::vector GetParentChain( const LIB_SYMBOL& aSymbol ) +static std::vector> GetParentChain( const LIB_SYMBOL& aSymbol ) { - std::vector chain( { aSymbol.SharedPtr() } ); + std::vector> chain( { aSymbol.SharedPtr() } ); while( chain.back()->IsAlias() ) { - LIB_SYMBOL_SPTR parent = chain.back()->GetParent().lock(); + std::shared_ptr parent = chain.back()->GetParent().lock(); chain.push_back( parent ); } @@ -583,7 +583,7 @@ static std::pair CheckSavingIntoOwnInheritance( LIB_SYMBOL_LIBRARY_M bool inDescendents = false; { - const std::vector parentChainFromUs = GetParentChain( aSymbol ); + const std::vector> parentChainFromUs = GetParentChain( aSymbol ); // Ignore the leaf symbol (0) - that must match for( size_t i = 1; i < parentChainFromUs.size(); ++i ) @@ -599,8 +599,8 @@ static std::pair CheckSavingIntoOwnInheritance( LIB_SYMBOL_LIBRARY_M { LIB_SYMBOL* targetSymbol = aLibMgr.GetAlias( aNewSymbolName, aNewLibraryName ); - const std::vector parentChainFromTarget = GetParentChain( *targetSymbol ); - const wxString oldSymbolName = aSymbol.GetName(); + const std::vector> parentChainFromTarget = GetParentChain( *targetSymbol ); + const wxString oldSymbolName = aSymbol.GetName(); // Ignore the leaf symbol - it'll match if we're saving the symbol // to the same name, and that would be OK @@ -644,7 +644,7 @@ static std::vector CheckForParentalChainConflicts( LIB_SYMBOL_LIBRARY_ else { // In a different library, check the whole chain - const std::vector parentChain = GetParentChain( aSymbol ); + const std::vector> parentChain = GetParentChain( aSymbol ); for( size_t i = 0; i < parentChain.size(); ++i ) { @@ -658,7 +658,7 @@ static std::vector CheckForParentalChainConflicts( LIB_SYMBOL_LIBRARY_ } else { - LIB_SYMBOL_SPTR chainSymbol = parentChain[i]; + std::shared_ptr chainSymbol = parentChain[i]; if( aLibMgr.SymbolExists( chainSymbol->GetName(), newLibraryName ) ) { conflicts.push_back( chainSymbol->GetName() ); @@ -701,7 +701,7 @@ public: bool DoSave( LIB_SYMBOL& symbol, const wxString& aNewSymName, const wxString& aNewLibName ) { - std::vector parentChain; + std::vector> parentChain; // If we're saving into the same library, we don't need to check the parental chain // because we can just keep the same parent symbol if( aNewLibName == symbol.GetLibId().GetLibNickname().wx_str() ) @@ -714,9 +714,8 @@ public: // Iterate backwards (i.e. from the root down) for( int i = (int) parentChain.size() - 1; i >= 0; --i ) { - LIB_SYMBOL_SPTR& oldSymbol = parentChain[i]; - - LIB_SYMBOL new_symbol( *oldSymbol ); + std::shared_ptr& oldSymbol = parentChain[i]; + LIB_SYMBOL new_symbol( *oldSymbol ); wxString newName; if( i == 0 ) @@ -1742,7 +1741,7 @@ void SYMBOL_EDIT_FRAME::UpdateSymbolMsgPanelInfo() if( m_symbol->IsAlias() ) { - LIB_SYMBOL_SPTR parent = m_symbol->GetParent().lock(); + std::shared_ptr parent = m_symbol->GetParent().lock(); msg = parent ? parent->GetName() : _( "Undefined!" ); AppendMsgPanel( _( "Parent" ), UnescapeString( msg ), 8 ); diff --git a/eeschema/symbol_library_manager.cpp b/eeschema/symbol_library_manager.cpp index 0b90d14960..60778b3d7e 100644 --- a/eeschema/symbol_library_manager.cpp +++ b/eeschema/symbol_library_manager.cpp @@ -1176,7 +1176,7 @@ bool LIB_BUFFER::HasDerivedSymbols( const wxString& aParentName ) const { if( entry->GetSymbol().IsAlias() ) { - LIB_SYMBOL_SPTR parent = entry->GetSymbol().GetParent().lock(); + std::shared_ptr parent = entry->GetSymbol().GetParent().lock(); // Check for inherited symbol without a valid parent. wxCHECK( parent, false ); @@ -1214,7 +1214,7 @@ size_t LIB_BUFFER::GetDerivedSymbolNames( const wxString& aSymbolName, wxArraySt const LIB_SYMBOL& symbol = entry->GetSymbol(); if( symbol.IsAlias() ) { - LIB_SYMBOL_SPTR parent = symbol.GetParent().lock(); + std::shared_ptr parent = symbol.GetParent().lock(); // Check for inherited symbol without a valid parent. wxCHECK2( parent, continue ); diff --git a/eeschema/symbol_viewer_frame.cpp b/eeschema/symbol_viewer_frame.cpp index 493f92d2b9..3c753f9789 100644 --- a/eeschema/symbol_viewer_frame.cpp +++ b/eeschema/symbol_viewer_frame.cpp @@ -449,9 +449,8 @@ void SYMBOL_VIEWER_FRAME::updatePreviewSymbol() view->Add( m_previewItem.get() ); wxString parentName; - std::shared_ptr parent = symbol->GetParent().lock(); - if( parent ) + if( std::shared_ptr parent = symbol->GetParent().lock() ) parentName = parent->GetName(); AppendMsgPanel( _( "Name" ), UnescapeString( m_previewItem->GetName() ) ); @@ -950,21 +949,10 @@ void SYMBOL_VIEWER_FRAME::CloseLibraryViewer( wxCommandEvent& event ) const BOX2I SYMBOL_VIEWER_FRAME::GetDocumentExtents( bool aIncludeAllVisible ) const { - LIB_SYMBOL* symbol = GetSelectedSymbol(); + if( LIB_SYMBOL* symbol = GetSelectedSymbol() ) + return symbol->GetUnitBoundingBox( m_unit, m_bodyStyle ); - if( !symbol ) - { - return BOX2I( VECTOR2I( -200, -200 ), VECTOR2I( 400, 400 ) ); - } - else - { - std::shared_ptr tmp = symbol->IsAlias() ? symbol->GetParent().lock() - : symbol->SharedPtr(); - - wxCHECK( tmp, BOX2I( VECTOR2I( -200, -200 ), VECTOR2I( 400, 400 ) ) ); - - return tmp->GetUnitBoundingBox( m_unit, m_bodyStyle ); - } + return BOX2I( VECTOR2I( -200, -200 ), VECTOR2I( 400, 400 ) ); } diff --git a/qa/tests/eeschema/test_lib_part.cpp b/qa/tests/eeschema/test_lib_part.cpp index 8204a3d9f6..08f3831a9c 100644 --- a/qa/tests/eeschema/test_lib_part.cpp +++ b/qa/tests/eeschema/test_lib_part.cpp @@ -607,12 +607,12 @@ BOOST_AUTO_TEST_CASE( Inheritance ) BOOST_CHECK( child->GetRootSymbol().get() == parent.get() ); BOOST_CHECK( grandChild->GetRootSymbol().get() == parent.get() ); - LIB_SYMBOL_SPTR parentRef = child->GetParent().lock(); + std::shared_ptr parentRef = child->GetParent().lock(); BOOST_CHECK( parentRef ); BOOST_CHECK( parentRef == parent->SharedPtr() ); BOOST_CHECK_EQUAL( parent->SharedPtr().use_count(), 3 ); - LIB_SYMBOL_SPTR childRef = grandChild->GetParent().lock(); + std::shared_ptr childRef = grandChild->GetParent().lock(); BOOST_CHECK( childRef ); BOOST_CHECK( childRef == child->SharedPtr() ); BOOST_CHECK_EQUAL( child->SharedPtr().use_count(), 3 );