From d962b062ca714888422ba0340e96a07a050fb483 Mon Sep 17 00:00:00 2001 From: Wayne Stambaugh Date: Sat, 8 Apr 2023 15:28:45 -0400 Subject: [PATCH] Fix crash when duplicating symbol unit with alternate body style. * Add check for valid symbol library link in annotation code to prevent crash even if the symbol was not defined correctly. This will assert in debug builds. * Fix broken pin comparisons when library symbols have units with alternate body style defined. * Remove some macros from LIB_SYMBOL comparison function which made debugging painful. * Exit LIB_SYMBOL comparison function as soon as non-equivalent comparison occurs. * Fix broken library symbol comparison due to name differences when checking for existing variants of the library symbol. Fixes https://gitlab.com/kicad/code/kicad/-/issues/14491 --- eeschema/lib_pin.cpp | 17 ++++++++++ eeschema/lib_pin.h | 6 ++++ eeschema/lib_symbol.cpp | 58 +++++++++++++++------------------ eeschema/sch_reference_list.cpp | 2 ++ eeschema/sch_screen.cpp | 8 +++++ 5 files changed, 60 insertions(+), 31 deletions(-) diff --git a/eeschema/lib_pin.cpp b/eeschema/lib_pin.cpp index fd13d19fc3..a457673a93 100644 --- a/eeschema/lib_pin.cpp +++ b/eeschema/lib_pin.cpp @@ -1416,6 +1416,23 @@ wxString LIB_PIN::GetItemDescription( UNITS_PROVIDER* aUnitsProvider ) const } +std::ostream& LIB_PIN::operator<<( std::ostream& aStream ) +{ + aStream << "LIB_PIN:" << std::endl + << " Name: \"" << m_name << "\"" << std::endl + << " Number: \"" << m_number << "\"" << std::endl + << " Position: " << m_position << std::endl + << " Length: " << m_length << std::endl + << " Orientation: " << PinOrientationName( m_orientation ) << std::endl + << " Shape: " << PinShapeGetText( m_shape ) << std::endl + << " Type: " << ElectricalPinTypeGetText( m_type ) << std::endl + << " Name Text Size: " << m_nameTextSize << std::endl + << " Number Text Size: " << m_numTextSize << std::endl; + + return aStream; +} + + #if defined(DEBUG) void LIB_PIN::Show( int nestLevel, std::ostream& os ) const diff --git a/eeschema/lib_pin.h b/eeschema/lib_pin.h index a94b1fc0fc..dddd9b83d9 100644 --- a/eeschema/lib_pin.h +++ b/eeschema/lib_pin.h @@ -27,6 +27,7 @@ #ifndef CLASS_PIN_H #define CLASS_PIN_H +#include #include #include @@ -279,6 +280,11 @@ protected: void printPinElectricalTypeName( const RENDER_SETTINGS* aSettings, VECTOR2I& aPosition, int aOrientation, bool aDimmed ); + bool operator==( const LIB_PIN& aRhs ) const { return compare( aRhs, EQUALITY ) == 0; } + bool operator<( const LIB_PIN& aRhs ) const { return compare( aRhs, EQUALITY ) < 0; } + bool operator>( const LIB_PIN& aRhs ) const { return compare( aRhs, EQUALITY ) > 0; } + std::ostream& operator<<( std::ostream& aStream ); + private: /** * @copydoc LIB_ITEM::compare() diff --git a/eeschema/lib_symbol.cpp b/eeschema/lib_symbol.cpp index 0ab37b2806..c5d970fa22 100644 --- a/eeschema/lib_symbol.cpp +++ b/eeschema/lib_symbol.cpp @@ -231,7 +231,6 @@ const LIB_SYMBOL& LIB_SYMBOL::operator=( const LIB_SYMBOL& aSymbol ) #define REPORT( msg ) { if( aReporter ) aReporter->Report( msg ); } #define ITEM_DESC( item ) ( item )->GetItemDescription( &unitsProvider ) -#define CHECKPOINT { if( retv && !aReporter ) return retv; } int LIB_SYMBOL::Compare( const LIB_SYMBOL& aRhs, int aCompareFlags, REPORTER* aReporter ) const { @@ -261,24 +260,22 @@ int LIB_SYMBOL::Compare( const LIB_SYMBOL& aRhs, int aCompareFlags, REPORTER* aR { retv = ( m_options == ENTRY_NORMAL ) ? -1 : 1; REPORT( _( "Power flag differs." ) ); + return retv; } - CHECKPOINT; - if( int tmp = m_unitCount - aRhs.m_unitCount ) { retv = tmp; REPORT( _( "Unit count differs." ) ); + return retv; } - CHECKPOINT; - - // Make sure shapes are sorted, but no need with fields and pins as we're going to - // match those up by id/name/number. + // Make sure shapes and pins are sorted. No need with fields as those are + // matched by id/name. std::set aShapes; std::set aFields; - std::set aPins; + std::set aPins; for( auto it = m_drawings.begin(); it != m_drawings.end(); ++it ) { @@ -292,7 +289,7 @@ int LIB_SYMBOL::Compare( const LIB_SYMBOL& aRhs, int aCompareFlags, REPORTER* aR std::set bShapes; std::set bFields; - std::set bPins; + std::set bPins; for( auto it = aRhs.m_drawings.begin(); it != aRhs.m_drawings.end(); ++it ) { @@ -308,6 +305,7 @@ int LIB_SYMBOL::Compare( const LIB_SYMBOL& aRhs, int aCompareFlags, REPORTER* aR { retv = tmp; REPORT( _( "Graphic item count differs." ) ); + return retv; } else { @@ -317,16 +315,22 @@ int LIB_SYMBOL::Compare( const LIB_SYMBOL& aRhs, int aCompareFlags, REPORTER* aR { retv = tmp2; REPORT( wxString::Format( _( "%s differs." ), ITEM_DESC( *aIt ) ) ); + return retv; } } } - CHECKPOINT; + if( int tmp = static_cast( aPins.size() - bPins.size() ) ) + { + retv = tmp; + REPORT( _( "Pin count differs." ) ); + return retv; + } for( const LIB_ITEM* aPinItem : aPins ) { const LIB_PIN* aPin = static_cast( aPinItem ); - const LIB_PIN* bPin = aRhs.GetPin( aPin->GetNumber() ); + const LIB_PIN* bPin = aRhs.GetPin( aPin->GetNumber(), aPin->GetUnit(), aPin->GetConvert() ); int tmp = 0; if( !bPin ) @@ -338,15 +342,10 @@ int LIB_SYMBOL::Compare( const LIB_SYMBOL& aRhs, int aCompareFlags, REPORTER* aR { retv = tmp; REPORT( wxString::Format( _( "Pin %s differs." ), aPin->GetNumber() ) ); + return retv; } } - if( int tmp = static_cast( aPins.size() - bPins.size() ) ) - { - retv = tmp; - REPORT( _( "Pin count differs." ) ); - } - for( const LIB_ITEM* aFieldItem : aFields ) { const LIB_FIELD* aField = static_cast( aFieldItem ); @@ -381,23 +380,22 @@ int LIB_SYMBOL::Compare( const LIB_SYMBOL& aRhs, int aCompareFlags, REPORTER* aR { retv = tmp; REPORT( wxString::Format( _( "%s field differs." ), aField->GetName( false ) ) ); + return retv; } } - CHECKPOINT; - if( int tmp = static_cast( aFields.size() - bFields.size() ) ) { retv = tmp; REPORT( _( "Field count differs." ) ); + return retv; } - CHECKPOINT; - if( int tmp = static_cast( m_fpFilters.GetCount() - aRhs.m_fpFilters.GetCount() ) ) { retv = tmp; REPORT( _( "Footprint filters differs." ) ); + return retv; } else { @@ -407,65 +405,63 @@ int LIB_SYMBOL::Compare( const LIB_SYMBOL& aRhs, int aCompareFlags, REPORTER* aR { retv = tmp2; REPORT( _( "Footprint filters differ." ) ); - break; + return retv; } } } - CHECKPOINT; - if( int tmp = m_description.Cmp( aRhs.m_description ) ) { retv = tmp; REPORT( _( "Symbol descriptions differ." ) ); + return retv; } - CHECKPOINT; - if( int tmp = m_keyWords.Cmp( aRhs.m_keyWords ) ) { retv = tmp; REPORT( _( "Symbol keywords differ." ) ); + return retv; } - CHECKPOINT; - if( int tmp = m_pinNameOffset - aRhs.m_pinNameOffset ) { retv = tmp; REPORT( _( "Symbol pin name offsets differ." ) ); + return retv; } - CHECKPOINT; - if( ( aCompareFlags & LIB_ITEM::COMPARE_FLAGS::ERC ) == 0 ) { if( m_showPinNames != aRhs.m_showPinNames ) { retv = ( m_showPinNames ) ? 1 : -1; REPORT( _( "Show pin names settings differ." ) ); + return retv; } if( m_showPinNumbers != aRhs.m_showPinNumbers ) { retv = ( m_showPinNumbers ) ? 1 : -1; REPORT( _( "Show pin numbers settings differ." ) ); + return retv; } if( m_includeInBom != aRhs.m_includeInBom ) { retv = ( m_includeInBom ) ? 1 : -1; REPORT( _( "Exclude from bill of materials settings differ." ) ); + return retv; } if( m_includeOnBoard != aRhs.m_includeOnBoard ) { retv = ( m_includeOnBoard ) ? 1 : -1; REPORT( _( "Exclude from board settings differ." ) ); + return retv; } } - CHECKPOINT; if( !aReporter ) { if( m_unitsLocked != aRhs.m_unitsLocked ) diff --git a/eeschema/sch_reference_list.cpp b/eeschema/sch_reference_list.cpp index 47a8c03262..83daa0ce3b 100644 --- a/eeschema/sch_reference_list.cpp +++ b/eeschema/sch_reference_list.cpp @@ -564,6 +564,8 @@ void SCH_REFERENCE_LIST::Annotate( bool aUseSheetNum, int aSheetIntervalId, int if( aStartAtCurrent && ref_unit.m_numRef > 0 ) minRefId = ref_unit.m_numRef; + wxCHECK( ref_unit.GetLibPart(), /* void */ ); + // Annotation of one part per package symbols (trivial case). if( ref_unit.GetLibPart()->GetUnitCount() <= 1 ) { diff --git a/eeschema/sch_screen.cpp b/eeschema/sch_screen.cpp index 0230f4d543..647dc01cb7 100644 --- a/eeschema/sch_screen.cpp +++ b/eeschema/sch_screen.cpp @@ -192,12 +192,20 @@ void SCH_SCREEN::Append( SCH_ITEM* aItem, bool aUpdateLibSymbol ) wxCHECK2( foundSymbol, continue ); + wxString tmp = symbol->GetLibSymbolRef()->GetName(); + + // Temporarily update the new symbol library symbol name so it + // doesn't fail on the name comparison below. + symbol->GetLibSymbolRef()->SetName( foundSymbol->GetName() ); + if( *foundSymbol == *symbol->GetLibSymbolRef() ) { newName = libSymbolName; + symbol->GetLibSymbolRef()->SetName( tmp ); break; } + symbol->GetLibSymbolRef()->SetName( tmp ); foundSymbol = nullptr; }