From 067fa6575632fa83559228d27d426d02fb680efb Mon Sep 17 00:00:00 2001 From: Wayne Stambaugh Date: Sun, 12 Dec 2021 17:01:13 -0500 Subject: [PATCH] Eeschema: fix a multitude of LIB_FIELD object comparison and index bugs. It is no longer acceptable to set a LIB_FIELD index to -1. This was trashing the sorting on load cause all kinds of false field comparison failures. There are now assertions when attempting to use -1 as a field index. Fixes https://gitlab.com/kicad/code/kicad/-/issues/9811 --- eeschema/lib_field.cpp | 61 ++++++++++---- eeschema/lib_field.h | 16 ++-- eeschema/lib_item.cpp | 4 +- eeschema/lib_item.h | 2 +- eeschema/lib_symbol.cpp | 81 ++++++++++++++++++- eeschema/lib_symbol.h | 25 +++++- .../sch_plugins/kicad/sch_sexpr_parser.cpp | 27 ++++--- .../sch_plugins/kicad/sch_sexpr_plugin.cpp | 43 +++++----- 8 files changed, 195 insertions(+), 64 deletions(-) diff --git a/eeschema/lib_field.cpp b/eeschema/lib_field.cpp index abe8b24f1b..65b88a16ff 100644 --- a/eeschema/lib_field.cpp +++ b/eeschema/lib_field.cpp @@ -39,24 +39,24 @@ #include -LIB_FIELD::LIB_FIELD( LIB_SYMBOL* aParent, int idfield ) : +LIB_FIELD::LIB_FIELD( LIB_SYMBOL* aParent, int aId ) : LIB_ITEM( LIB_FIELD_T, aParent ) { - Init( idfield ); + Init( aId ); } -LIB_FIELD::LIB_FIELD( int idfield ) : +LIB_FIELD::LIB_FIELD( int aId ) : LIB_ITEM( LIB_FIELD_T, nullptr ) { - Init( idfield ); + Init( aId ); } -LIB_FIELD::LIB_FIELD( int aID, const wxString& aName ) : +LIB_FIELD::LIB_FIELD( int aId, const wxString& aName ) : LIB_ITEM( LIB_FIELD_T, nullptr ) { - Init( aID ); + Init( aId ); m_name = aName; } @@ -79,23 +79,32 @@ LIB_FIELD& LIB_FIELD::operator=( const LIB_FIELD& field ) } -void LIB_FIELD::Init( int id ) +void LIB_FIELD::Init( int aId ) { - m_id = id; + wxCHECK2( aId >= 0, aId = MANDATORY_FIELDS ); + + m_id = aId; SetTextAngle( TEXT_ANGLE_HORIZ ); // constructor already did this. // Fields in RAM must always have names, because we are trying to get less dependent on // field ids and more dependent on names. Plus assumptions are made in the field editors. - m_name = TEMPLATE_FIELDNAME::GetDefaultFieldName( id ); + m_name = TEMPLATE_FIELDNAME::GetDefaultFieldName( aId ); // By contrast, VALUE and REFERENCE are are always constructed as initially visible, and // template fieldsnames' initial visibility is controlled by the template fieldname config. - if( id == DATASHEET_FIELD || id == FOOTPRINT_FIELD ) + if( aId == DATASHEET_FIELD || aId == FOOTPRINT_FIELD ) SetVisible( false ); } +void LIB_FIELD::SetId( int aId ) +{ + wxCHECK2( aId >= 0, aId = MANDATORY_FIELDS ); + m_id = aId; +} + + int LIB_FIELD::GetPenWidth() const { return GetEffectiveTextPenWidth(); @@ -181,13 +190,35 @@ int LIB_FIELD::compare( const LIB_ITEM& aOther, LIB_ITEM::COMPARE_FLAGS aCompare const LIB_FIELD* tmp = ( LIB_FIELD* ) &aOther; - if( m_id != tmp->m_id ) - return m_id - tmp->m_id; + // Equality test will vary depending whether or not the field is mandatory. Otherwise, + // sorting is done by ordinal. + if( aCompareFlags & LIB_ITEM::COMPARE_FLAGS::EQUALITY ) + { + // Mandatory fields have fixed ordinals and their names can vary due to translated field + // names. Optional fields have fixed names and their ordinals can vary. + if( IsMandatory() ) + { + if( m_id != tmp->m_id ) + return m_id - tmp->m_id; + } + else + { + retv = m_name.Cmp( tmp->m_name ); - int result = GetText().CmpNoCase( tmp->GetText() ); + if( retv ) + return retv; + } + } + else + { + if( m_id != tmp->m_id ) + return m_id - tmp->m_id; + } - if( result != 0 ) - return result; + retv = GetText().CmpNoCase( tmp->GetText() ); + + if( retv != 0 ) + return retv; if( GetTextPos().x != tmp->GetTextPos().x ) return GetTextPos().x - tmp->GetTextPos().x; diff --git a/eeschema/lib_field.h b/eeschema/lib_field.h index 93e77474e4..696507c2b0 100644 --- a/eeschema/lib_field.h +++ b/eeschema/lib_field.h @@ -59,11 +59,11 @@ class SCH_LEGACY_PLUGIN_CACHE; class LIB_FIELD : public LIB_ITEM, public EDA_TEXT { public: - LIB_FIELD( int idfield = 2 ); + LIB_FIELD( int aId = 2 ); - LIB_FIELD( int aID, const wxString& aName ); + LIB_FIELD( int aId, const wxString& aName ); - LIB_FIELD( LIB_SYMBOL* aParent, int idfield = 2 ); + LIB_FIELD( LIB_SYMBOL* aParent, int aId = 2 ); // Do not create a copy constructor. The one generated by the compiler is adequate. @@ -82,7 +82,7 @@ public: /** * Object constructor initialization helper. */ - void Init( int idfield ); + void Init( int aId ); /** * Return the field name. @@ -91,7 +91,7 @@ public: * names. * * The user definable fields will return FieldN where N is the ID of the field when the - * m_name member is empyt unless false is passed to \a aUseDefaultName. + * m_name member is empty unless false is passed to \a aUseDefaultName. */ wxString GetName( bool aUseDefaultName = true ) const; @@ -113,7 +113,7 @@ public: void SetName( const wxString& aName ); int GetId() const { return m_id; } - void SetId( int aId ) { m_id = aId; } + void SetId( int aId ); int GetPenWidth() const override; @@ -206,8 +206,8 @@ private: friend class SCH_LEGACY_PLUGIN_CACHE; // Required to access m_name. - int m_id; ///< @see enum MANDATORY_FIELD_T - wxString m_name; ///< Name (not the field text value itself, that is .m_Text) + int m_id; ///< @see enum MANDATORY_FIELD_T + wxString m_name; ///< Name (not the field text value itself, that is #EDA_TEXT::m_Text) }; #endif // CLASS_LIBENTRY_FIELDS_H diff --git a/eeschema/lib_item.cpp b/eeschema/lib_item.cpp index c3d132b050..4ced5f420d 100644 --- a/eeschema/lib_item.cpp +++ b/eeschema/lib_item.cpp @@ -2,7 +2,7 @@ * This program source code file is part of KiCad, a free EDA CAD application. * * Copyright (C) 2015 Jean-Pierre Charras, jaen-pierre.charras at wanadoo.fr - * Copyright (C) 2015 Wayne Stambaugh + * Copyright (C) 2015 Wayne Stambaugh * Copyright (C) 2004-2021 KiCad Developers, see change_log.txt for contributors. * * This program is free software; you can redistribute it and/or @@ -85,7 +85,7 @@ bool LIB_ITEM::operator==( const LIB_ITEM& aOther ) const if( Type() != aOther.Type() ) return false; - return compare( aOther ) == 0; + return compare( aOther, LIB_ITEM::COMPARE_FLAGS::EQUALITY ) == 0; } diff --git a/eeschema/lib_item.h b/eeschema/lib_item.h index a54834927a..b30f6507cc 100644 --- a/eeschema/lib_item.h +++ b/eeschema/lib_item.h @@ -77,7 +77,7 @@ public: * - UNIT This compare flag ignores unit and convert and pin number information when * comparing #LIB_ITEM objects for unit comparison. */ - enum COMPARE_FLAGS : int { NORMAL = 0x00, UNIT = 0x01 }; + enum COMPARE_FLAGS : int { NORMAL = 0x00, UNIT = 0x01, EQUALITY = 0x02 }; /** * Provide a user-consumable name of the object type. Perform localization when diff --git a/eeschema/lib_symbol.cpp b/eeschema/lib_symbol.cpp index 27cbf5df26..ec4b7e5aec 100644 --- a/eeschema/lib_symbol.cpp +++ b/eeschema/lib_symbol.cpp @@ -209,7 +209,7 @@ const LIB_SYMBOL& LIB_SYMBOL::operator=( const LIB_SYMBOL& aSymbol ) } -int LIB_SYMBOL::Compare( const LIB_SYMBOL& aRhs ) const +int LIB_SYMBOL::Compare( const LIB_SYMBOL& aRhs, LIB_ITEM::COMPARE_FLAGS aCompareFlags ) const { if( m_me == aRhs.m_me ) return 0; @@ -252,7 +252,19 @@ int LIB_SYMBOL::Compare( const LIB_SYMBOL& aRhs ) const if( lhsItem->Type() != rhsItem->Type() ) return lhsItem->Type() - rhsItem->Type(); - retv = lhsItem->compare( *rhsItem ); + // Non-mandatory fields are a special case. They can have different ordinal numbers + // and are compared separately below. + if( lhsItem->Type() == LIB_FIELD_T ) + { + const LIB_FIELD* lhsField = static_cast( lhsItem ); + + if( lhsField->IsMandatory() ) + retv = lhsItem->compare( *rhsItem, aCompareFlags ); + } + else + { + retv = lhsItem->compare( *rhsItem, aCompareFlags ); + } if( retv ) return retv; @@ -261,6 +273,28 @@ int LIB_SYMBOL::Compare( const LIB_SYMBOL& aRhs ) const ++rhsItemIt; } + // Compare the optional fields. + for( const LIB_ITEM& item : m_drawings[ LIB_FIELD_T ] ) + { + const LIB_FIELD* field = dynamic_cast( &item ); + + wxCHECK2( field, continue ); + + // Mandatory fields were already compared above. + if( field->IsMandatory() ) + continue; + + const LIB_FIELD* foundField = aRhs.FindField( field->GetName() ); + + if( foundField == nullptr ) + return 1; + + retv = item.compare( static_cast( *foundField ), aCompareFlags ); + + if( retv ) + return retv; + } + if( m_fpFilters.GetCount() != aRhs.m_fpFilters.GetCount() ) return m_fpFilters.GetCount() - aRhs.m_fpFilters.GetCount(); @@ -342,7 +376,8 @@ std::unique_ptr< LIB_SYMBOL > LIB_SYMBOL::Flatten() const // Copy the parent. retv.reset( new LIB_SYMBOL( *parent.get() ) ); - retv->SetName( m_name ); + retv->m_name = m_name; + retv->SetLibId( m_libId ); // Now add the inherited part mandatory field (this) information. for( int i = 0; i < MANDATORY_FIELDS; i++ ) @@ -386,6 +421,7 @@ std::unique_ptr< LIB_SYMBOL > LIB_SYMBOL::Flatten() const retv->SetKeyWords( m_keyWords.IsEmpty() ? parent->GetKeyWords() : m_keyWords ); retv->SetDescription( m_description.IsEmpty() ? parent->GetDescription() : m_description ); retv->SetFPFilters( m_fpFilters.IsEmpty() ? parent->GetFPFilters() : m_fpFilters ); + retv->UpdateFieldOrdinals(); } else { @@ -1032,6 +1068,45 @@ LIB_FIELD& LIB_SYMBOL::GetDatasheetField() } +int LIB_SYMBOL::UpdateFieldOrdinals() +{ + int retv = 0; + int lastOrdinal = MANDATORY_FIELDS; + + for( LIB_ITEM& item : m_drawings[ LIB_FIELD_T ] ) + { + LIB_FIELD* field = dynamic_cast( &item ); + + wxCHECK2( field, continue ); + + // Mandatory fields were already resolved always have the same ordinal values. + if( field->IsMandatory() ) + continue; + + if( field->GetId() != lastOrdinal ) + { + field->SetId( lastOrdinal ); + retv += 1; + } + + lastOrdinal += 1; + } + + return retv; +} + + +int LIB_SYMBOL::GetNextAvailableFieldId() const +{ + int retv = MANDATORY_FIELDS; + + while( GetFieldById( retv ) ) + retv += 1; + + return retv; +} + + void LIB_SYMBOL::SetOffset( const wxPoint& aOffset ) { for( LIB_ITEM& item : m_drawings ) diff --git a/eeschema/lib_symbol.h b/eeschema/lib_symbol.h index 6c83b10d5c..cb837eae70 100644 --- a/eeschema/lib_symbol.h +++ b/eeschema/lib_symbol.h @@ -292,6 +292,17 @@ public: /** Return reference to the datasheet field. */ LIB_FIELD& GetDatasheetField(); + /** + * Order optional field indices. + * + * It's possible when calling #LIB_SYMBOL::Flatten that there can be gaps and/or duplicate + * optional field indices. This method correctly orders the indices so there are no gaps + * and/or duplicate indices. + */ + int UpdateFieldOrdinals(); + + int GetNextAvailableFieldId() const; + /** * Print symbol. * @@ -601,11 +612,19 @@ public: * 1 if this symbol is greater than \a aRhs * 0 if this symbol is the same as \a aRhs */ - int Compare( const LIB_SYMBOL& aRhs ) const; + int Compare( const LIB_SYMBOL& aRhs, + LIB_ITEM::COMPARE_FLAGS aCompareFlags = LIB_ITEM::COMPARE_FLAGS::NORMAL ) const; bool operator==( const LIB_SYMBOL* aSymbol ) const { return this == aSymbol; } - bool operator==( const LIB_SYMBOL& aSymbol ) const { return Compare( aSymbol ) == 0; } - bool operator!=( const LIB_SYMBOL& aSymbol ) const { return Compare( aSymbol ) != 0; } + bool operator==( const LIB_SYMBOL& aSymbol ) const + { + return Compare( aSymbol, LIB_ITEM::COMPARE_FLAGS::EQUALITY ) == 0; + } + + bool operator!=( const LIB_SYMBOL& aSymbol ) const + { + return Compare( aSymbol, LIB_ITEM::COMPARE_FLAGS::EQUALITY ) != 0; + } const LIB_SYMBOL& operator=( const LIB_SYMBOL& aSymbol ); diff --git a/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp b/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp index 9c42562583..9e15c57f74 100644 --- a/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp +++ b/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp @@ -236,17 +236,23 @@ LIB_SYMBOL* SCH_SEXPR_PARSER::ParseSymbol( LIB_SYMBOL_MAP& aSymbolLibMap, int aF if( field ) { - // It would appear that at some point we allowed duplicate ids to slip through - // when writing files. The easiest (and most complete) solution is to disallow - // multiple instances of the same id (for all files since the source of the error - // *might* in fact be hand-edited files). - // - // While no longer used, -1 is still a valid id for user field. It gets converted - // to the next unused number on save. + // Due to an bug when in #LIB_SYMBOL::Flatten, duplicate ids slipped through + // when writing files. This section replaces duplicate #LIB_FIELD indices on + // load. if( fieldIDsRead.count( field->GetId() ) ) - field->SetId( -1 ); + { + int nextAvailableId = field->GetId() + 1; + + while( fieldIDsRead.count( nextAvailableId ) ) + nextAvailableId += 1; + + fieldIDsRead.insert( nextAvailableId ); + field->SetId( nextAvailableId ); + } else if( field ) + { fieldIDsRead.insert( field->GetId() ); + } } break; @@ -2099,6 +2105,7 @@ void SCH_SEXPR_PARSER::ParseSchematic( SCH_SHEET* aSheet, bool aIsCopyableOnly, { // Dummy map. No derived symbols are allowed in the library cache. LIB_SYMBOL_MAP symbolLibMap; + LIB_SYMBOL* symbol; for( token = NextTok(); token != T_RIGHT; token = NextTok() ) { @@ -2110,7 +2117,9 @@ void SCH_SEXPR_PARSER::ParseSchematic( SCH_SHEET* aSheet, bool aIsCopyableOnly, switch( token ) { case T_symbol: - screen->AddLibSymbol( ParseSymbol( symbolLibMap, m_requiredVersion ) ); + symbol = ParseSymbol( symbolLibMap, m_requiredVersion ); + symbol->UpdateFieldOrdinals(); + screen->AddLibSymbol( symbol ); break; default: diff --git a/eeschema/sch_plugins/kicad/sch_sexpr_plugin.cpp b/eeschema/sch_plugins/kicad/sch_sexpr_plugin.cpp index 3b679d49a7..7fecb12a93 100644 --- a/eeschema/sch_plugins/kicad/sch_sexpr_plugin.cpp +++ b/eeschema/sch_plugins/kicad/sch_sexpr_plugin.cpp @@ -445,8 +445,7 @@ class SCH_SEXPR_PLUGIN_CACHE static void saveSymbolDrawItem( LIB_ITEM* aItem, OUTPUTFORMATTER& aFormatter, int aNestLevel ); - static void saveField( LIB_FIELD* aField, OUTPUTFORMATTER& aFormatter, int& aNextFreeFieldId, - int aNestLevel ); + static void saveField( LIB_FIELD* aField, OUTPUTFORMATTER& aFormatter, int aNestLevel ); static void savePin( LIB_PIN* aPin, OUTPUTFORMATTER& aFormatter, int aNestLevel = 0 ); static void saveText( LIB_TEXT* aText, OUTPUTFORMATTER& aFormatter, int aNestLevel = 0 ); @@ -1797,15 +1796,18 @@ void SCH_SEXPR_PLUGIN_CACHE::SaveSymbol( LIB_SYMBOL* aSymbol, OUTPUTFORMATTER& a aSymbol->GetFields( fields ); for( LIB_FIELD* field : fields ) - saveField( field, aFormatter, nextFreeFieldId, aNestLevel + 1 ); + saveField( field, aFormatter, aNestLevel + 1 ); + + nextFreeFieldId = aSymbol->GetNextAvailableFieldId(); // @todo At some point in the future the lock status (all units interchangeable) should // be set deterministically. For now a custom lock property is used to preserve the // locked flag state. if( aSymbol->UnitsLocked() ) { - LIB_FIELD locked( -1, "ki_locked" ); - saveField( &locked, aFormatter, nextFreeFieldId, aNestLevel + 1 ); + LIB_FIELD locked( nextFreeFieldId, "ki_locked" ); + saveField( &locked, aFormatter, aNestLevel + 1 ); + nextFreeFieldId += 1; } saveDcmInfoAsFields( aSymbol, aFormatter, nextFreeFieldId, aNestLevel ); @@ -1861,7 +1863,9 @@ void SCH_SEXPR_PLUGIN_CACHE::SaveSymbol( LIB_SYMBOL* aSymbol, OUTPUTFORMATTER& a aSymbol->GetFields( fields ); for( LIB_FIELD* field : fields ) - saveField( field, aFormatter, nextFreeFieldId, aNestLevel + 1 ); + saveField( field, aFormatter, aNestLevel + 1 ); + + nextFreeFieldId = aSymbol->GetNextAvailableFieldId(); saveDcmInfoAsFields( aSymbol, aFormatter, nextFreeFieldId, aNestLevel ); } @@ -1877,18 +1881,20 @@ void SCH_SEXPR_PLUGIN_CACHE::saveDcmInfoAsFields( LIB_SYMBOL* aSymbol, OUTPUTFOR if( !aSymbol->GetKeyWords().IsEmpty() ) { - LIB_FIELD keywords( -1, wxString( "ki_keywords" ) ); + LIB_FIELD keywords( aNextFreeFieldId, wxString( "ki_keywords" ) ); keywords.SetVisible( false ); keywords.SetText( aSymbol->GetKeyWords() ); - saveField( &keywords, aFormatter, aNextFreeFieldId, aNestLevel + 1 ); + saveField( &keywords, aFormatter, aNestLevel + 1 ); + aNextFreeFieldId += 1; } if( !aSymbol->GetDescription().IsEmpty() ) { - LIB_FIELD description( -1, wxString( "ki_description" ) ); + LIB_FIELD description( aNextFreeFieldId, wxString( "ki_description" ) ); description.SetVisible( false ); description.SetText( aSymbol->GetDescription() ); - saveField( &description, aFormatter, aNextFreeFieldId, aNestLevel + 1 ); + saveField( &description, aFormatter, aNestLevel + 1 ); + aNextFreeFieldId += 1; } wxArrayString fpFilters = aSymbol->GetFPFilters(); @@ -1908,10 +1914,11 @@ void SCH_SEXPR_PLUGIN_CACHE::saveDcmInfoAsFields( LIB_SYMBOL* aSymbol, OUTPUTFOR tmp += " " + curr_filter; } - LIB_FIELD description( -1, wxString( "ki_fp_filters" ) ); + LIB_FIELD description( aNextFreeFieldId, wxString( "ki_fp_filters" ) ); description.SetVisible( false ); description.SetText( tmp ); - saveField( &description, aFormatter, aNextFreeFieldId, aNestLevel + 1 ); + saveField( &description, aFormatter, aNestLevel + 1 ); + aNextFreeFieldId += 1; } } @@ -1980,22 +1987,12 @@ void SCH_SEXPR_PLUGIN_CACHE::saveSymbolDrawItem( LIB_ITEM* aItem, OUTPUTFORMATTE void SCH_SEXPR_PLUGIN_CACHE::saveField( LIB_FIELD* aField, OUTPUTFORMATTER& aFormatter, - int& aNextFreeFieldId, int aNestLevel ) + int aNestLevel ) { wxCHECK_RET( aField && aField->Type() == LIB_FIELD_T, "Invalid LIB_FIELD object." ); wxString fieldName = aField->GetName(); - if( aField->GetId() == -1 /* undefined ID */ ) - { - aField->SetId( aNextFreeFieldId ); - aNextFreeFieldId += 1; - } - else if( aField->GetId() >= aNextFreeFieldId ) - { - aNextFreeFieldId = aField->GetId() + 1; - } - if( aField->GetId() >= 0 && aField->GetId() < MANDATORY_FIELDS ) fieldName = TEMPLATE_FIELDNAME::GetDefaultFieldName( aField->GetId(), false );