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 857c408b25)

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<LIB_SYMBOL> parent = m_libEntry->GetParent().lock() )
         {
             std::vector<SCH_FIELD*> 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<LIB_SYMBOL> rootSymbol = m_libEntry->GetParent().lock() )
         {
             wxString parentName = UnescapeString( rootSymbol->GetName() );
             int selection = m_inheritanceSelectCombo->FindString( parentName );

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<LIB_SYMBOL> parent = m_symbol->GetParent().lock() )
+        m_parentSymbolReadOnly->SetValue( UnescapeString( parent->GetName() ) );
 
     for( int i = 0; i < MANDATORY_FIELD_COUNT; ++i )
     {

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<LIB_SYMBOL> parent;
+    LIB_SYMBOL*                 symbolToPlot = symbol;
 
     // if the symbol is an alias, then the draw items are stored in the root symbol
     if( symbol->IsAlias() )

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() )

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<LIB_SYMBOL> 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> 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<LIB_SYMBOL> 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<LIB_SYMBOL> 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<LIB_SYMBOL> 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<LIB_SYMBOL> 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<LIB_SYMBOL> 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<SCH_PIN*> LIB_SYMBOL::GetPins( int aUnit, int aBodyStyle ) const
 {
+    if( std::shared_ptr<LIB_SYMBOL> parent = m_parent.lock() )
+        return parent->GetPins( aUnit, aBodyStyle );
+
     std::vector<SCH_PIN*> pins;
 
     /* Notes:
@@ -874,10 +850,7 @@ std::vector<SCH_PIN*> 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<LIB_SYMBOL> 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<LIB_SYMBOL> 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<LIB_SYMBOL> 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<const SCH_PIN*>( &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<LIB_SYMBOL> parent = m_parent.lock() )
         return parent->GetUnitCount();
 
     return m_unitCount;

eeschema/lib_symbol.h

@@ -49,8 +49,6 @@ namespace KIFONT
 }
 
 
-typedef std::shared_ptr<LIB_SYMBOL>       LIB_SYMBOL_SPTR;      ///< shared pointer to LIB_SYMBOL
-typedef std::weak_ptr<LIB_SYMBOL>         LIB_SYMBOL_REF;       ///< weak pointer to LIB_SYMBOL
 typedef MULTIVECTOR<SCH_ITEM, SCH_SHAPE_T, SCH_PIN_T> 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<LIB_SYMBOL> 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<LIB_SYMBOL>& GetParent() { return m_parent; }
+    const std::weak_ptr<LIB_SYMBOL>& 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<LIB_SYMBOL> 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<LIB_SYMBOL> 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<LIB_SYMBOL> 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<LIB_SYMBOL> 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<LIB_SYMBOL> m_me;
+    std::weak_ptr<LIB_SYMBOL>   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;

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_SYMBOL> lib_match_parent;
 
             // If it's a derive symbol, use the parent symbol to perform the pin test.
             if( lib_match && lib_match->IsAlias() )

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<LIB_SYMBOL_SPTR> GetParentChain( const LIB_SYMBOL& aSymbol )
+static std::vector<std::shared_ptr<LIB_SYMBOL>> GetParentChain( const LIB_SYMBOL& aSymbol )
 {
-    std::vector<LIB_SYMBOL_SPTR> chain( { aSymbol.SharedPtr() } );
+    std::vector<std::shared_ptr<LIB_SYMBOL>> chain( { aSymbol.SharedPtr() } );
 
     while( chain.back()->IsAlias() )
     {
-        LIB_SYMBOL_SPTR parent = chain.back()->GetParent().lock();
+        std::shared_ptr<LIB_SYMBOL> parent = chain.back()->GetParent().lock();
         chain.push_back( parent );
     }
 
@@ -583,7 +583,7 @@ static std::pair<bool, bool> CheckSavingIntoOwnInheritance( LIB_SYMBOL_LIBRARY_M
     bool inDescendents = false;
 
     {
-        const std::vector<LIB_SYMBOL_SPTR> parentChainFromUs = GetParentChain( aSymbol );
+        const std::vector<std::shared_ptr<LIB_SYMBOL>> 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<bool, bool> CheckSavingIntoOwnInheritance( LIB_SYMBOL_LIBRARY_M
 
     {
         LIB_SYMBOL* targetSymbol = aLibMgr.GetAlias( aNewSymbolName, aNewLibraryName );
-        const std::vector<LIB_SYMBOL_SPTR> parentChainFromTarget = GetParentChain( *targetSymbol );
-        const wxString                     oldSymbolName = aSymbol.GetName();
+        const std::vector<std::shared_ptr<LIB_SYMBOL>> 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<wxString> CheckForParentalChainConflicts( LIB_SYMBOL_LIBRARY_
     else
     {
         // In a different library, check the whole chain
-        const std::vector<LIB_SYMBOL_SPTR> parentChain = GetParentChain( aSymbol );
+        const std::vector<std::shared_ptr<LIB_SYMBOL>> parentChain = GetParentChain( aSymbol );
 
         for( size_t i = 0; i < parentChain.size(); ++i )
         {
@@ -658,7 +658,7 @@ static std::vector<wxString> CheckForParentalChainConflicts( LIB_SYMBOL_LIBRARY_
             }
             else
             {
-                LIB_SYMBOL_SPTR chainSymbol = parentChain[i];
+                std::shared_ptr<LIB_SYMBOL> 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<LIB_SYMBOL_SPTR> parentChain;
+        std::vector<std::shared_ptr<LIB_SYMBOL>> 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<LIB_SYMBOL>& 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<LIB_SYMBOL> parent = m_symbol->GetParent().lock();
 
         msg = parent ? parent->GetName() : _( "Undefined!" );
         AppendMsgPanel( _( "Parent" ), UnescapeString( msg ), 8 );

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<LIB_SYMBOL> 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<LIB_SYMBOL> parent = symbol.GetParent().lock();
 
             // Check for inherited symbol without a valid parent.
             wxCHECK2( parent, continue );

eeschema/symbol_viewer_frame.cpp

@@ -449,9 +449,8 @@ void SYMBOL_VIEWER_FRAME::updatePreviewSymbol()
         view->Add( m_previewItem.get() );
 
         wxString parentName;
-        std::shared_ptr<LIB_SYMBOL> parent  = symbol->GetParent().lock();
 
-        if( parent )
+        if( std::shared_ptr<LIB_SYMBOL> 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<LIB_SYMBOL> 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 ) );
 }
 
 

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<LIB_SYMBOL> 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<LIB_SYMBOL> childRef = grandChild->GetParent().lock();
     BOOST_CHECK( childRef );
     BOOST_CHECK( childRef == child->SharedPtr() );
     BOOST_CHECK_EQUAL( child->SharedPtr().use_count(), 3 );