kicad-developers team mailing list archive

[Maciej Suminski <maciej.suminski@xxxxxxx>]

Hi,

I think this is a question to Wayne, but if others have any idea what
may go wrong, do not hesitate to reply.

I have noticed that part name and library values are duplicated in a few
places in LIB_PART class and it is possible to set them individually. It
means there is a way to set each of them to a different value,
potentially causing more problems.

With the attached patches, there is only one method to set all fields to
the same value. It is called in all places where the fields were
modified directly.

My main concern is related to LIB_ID: are there any cases where the
LIB_ID name/library fields should be different than what is stored in
the LIB_PART fields?

Regards,
Orson

>From 9f0cea0c3dc73c73e034aacf16db2efca33975f3 Mon Sep 17 00:00:00 2001
From: Maciej Suminski <maciej.suminski@xxxxxxx>
Date: Sun, 12 Nov 2017 21:49:38 +0100
Subject: [PATCH 1/4] Unified way of setting LIB_PART name

LIB_PART name is stored in three places that might be changed
independently:
- the first LIB_ALIAS in m_aliases
- LIB_FIELD with VALUE ID
- m_name wxString field

This is potentially leads to an incoherent LIB_PART state. To prevent
this, all fields are changed using only one method: LIB_PART::SetName().

LIB_PART::m_name has been removed as the same information is available
in two other variables.
---
 eeschema/class_libentry.cpp    | 34 ++++++++++------------------------
 eeschema/class_libentry.h      |  3 +--
 eeschema/sch_legacy_plugin.cpp | 15 +++++----------
 3 files changed, 16 insertions(+), 36 deletions(-)

diff --git a/eeschema/class_libentry.cpp b/eeschema/class_libentry.cpp
index 44f95588b..7cba8ac47 100644
--- a/eeschema/class_libentry.cpp
+++ b/eeschema/class_libentry.cpp
@@ -180,7 +180,6 @@ LIB_PART::LIB_PART( const wxString& aName, PART_LIB* aLibrary ) :
     EDA_ITEM( LIB_PART_T ),
     m_me( this, null_deleter() )
 {
-    m_name                = aName;
     m_library             = aLibrary;
     m_dateModified        = 0;
     m_unitCount           = 1;
@@ -190,21 +189,16 @@ LIB_PART::LIB_PART( const wxString& aName, PART_LIB* aLibrary ) :
     m_showPinNumbers      = true;
     m_showPinNames        = true;
 
-    m_libId.SetLibItemName( aName, false );
-
-    // Create the default alias if the name parameter is not empty.
-    if( !aName.IsEmpty() )
-        m_aliases.push_back( new LIB_ALIAS( aName, this ) );
+    wxASSERT( !aName.IsEmpty() );
 
     // Add the MANDATORY_FIELDS in RAM only.  These are assumed to be present
     // when the field editors are invoked.
-    LIB_FIELD* value = new LIB_FIELD( this, VALUE );
-    value->SetText( aName );
-    m_drawings[LIB_FIELD_T].push_back( value );
-
+    m_drawings[LIB_FIELD_T].push_back( new LIB_FIELD( this, VALUE ) );
     m_drawings[LIB_FIELD_T].push_back( new LIB_FIELD( this, REFERENCE ) );
     m_drawings[LIB_FIELD_T].push_back( new LIB_FIELD( this, FOOTPRINT ) );
     m_drawings[LIB_FIELD_T].push_back( new LIB_FIELD( this, DATASHEET ) );
+
+    SetName( aName );
 }
 
 
@@ -215,7 +209,6 @@ LIB_PART::LIB_PART( LIB_PART& aPart, PART_LIB* aLibrary ) :
     LIB_ITEM* newItem;
 
     m_library             = aLibrary;
-    m_name                = aPart.m_name;
     m_FootprintList       = aPart.m_FootprintList;
     m_unitCount           = aPart.m_unitCount;
     m_unitsLocked         = aPart.m_unitsLocked;
@@ -305,7 +298,6 @@ wxString LIB_PART::SubReference( int aUnit, bool aAddSeparator )
 
 void LIB_PART::SetName( const wxString& aName )
 {
-    m_name = aName;
     GetValueField().SetText( aName );
 
     // The LIB_ALIAS that is the LIB_PART name has to be created so create it.
@@ -897,23 +889,16 @@ bool LIB_PART::Load( LINE_READER& aLineReader, wxString& aErrorMsg )
     m_showPinNames = ( drawname == 'N' ) ? false : true;
 
     // Copy part name and prefix.
-    LIB_FIELD& value = GetValueField();
-
     if( componentName[0] != '~' )
     {
-        m_name = FROM_UTF8( componentName );
-        value.SetText( m_name );
+        SetName( FROM_UTF8( componentName ) );
     }
     else
     {
-        m_name = FROM_UTF8( &componentName[1] );
-        value.SetText( m_name );
-        value.SetVisible( false );
+        SetName( FROM_UTF8( &componentName[1] ) );
+        GetValueField().SetVisible( false );
     }
 
-    // Add the root alias to the alias list.
-    m_aliases.push_back( new LIB_ALIAS( m_name, this ) );
-
     LIB_FIELD& reference = GetReferenceField();
 
     if( strcmp( prefix, "~" ) == 0 )
@@ -1111,7 +1096,7 @@ bool LIB_PART::LoadField( LINE_READER& aLineReader, wxString& aErrorMsg )
         *fixedField = *field;
 
         if( field->GetId() == VALUE )
-            m_name = field->GetText();
+            SetName( field->GetText() );
 
         delete field;
     }
@@ -1708,6 +1693,7 @@ void LIB_PART::SetAliases( const wxArrayString& aAliasList )
 {
     wxCHECK_RET( !m_library,
                  wxT( "Part aliases cannot be changed when they are owned by a library." ) );
+    wxCHECK_RET( !aAliasList.IsEmpty(), wxT( "Alias list cannot be empty" ) );
 
     if( aAliasList == GetAliasNames() )
         return;
@@ -1782,7 +1768,7 @@ LIB_ALIAS* LIB_PART::RemoveAlias( LIB_ALIAS* aAlias )
         wxLogTrace( traceSchLibMem,
                     wxT( "%s: part:'%s', alias:'%s', alias count %llu, reference count %ld." ),
                     GetChars( wxString::FromAscii( __WXFUNCTION__ ) ),
-                    GetChars( m_name ),
+                    GetChars( GetName() ),
                     GetChars( aAlias->GetName() ),
                     (long long unsigned) m_aliases.size(),
                     m_me.use_count() );
diff --git a/eeschema/class_libentry.h b/eeschema/class_libentry.h
index 5decec9c5..1301faf1e 100644
--- a/eeschema/class_libentry.h
+++ b/eeschema/class_libentry.h
@@ -231,7 +231,6 @@ class LIB_PART : public EDA_ITEM
     friend class SCH_LEGACY_PLUGIN_CACHE;
 
     PART_SPTR           m_me;               ///< http://www.boost.org/doc/libs/1_55_0/libs/smart_ptr/sp_techniques.html#weak_without_shared
-    wxString            m_name;
     LIB_ID              m_libId;
     int                 m_pinNameOffset;    ///< The offset in mils to draw the pin name.  Set to 0
                                             ///< to draw the pin name above the pin.
@@ -280,7 +279,7 @@ public:
 
     virtual void SetName( const wxString& aName );
 
-    const wxString& GetName() const { return m_name; }
+    const wxString& GetName() const { return m_aliases[0]->GetName(); }
 
     const LIB_ID& GetLibId() const { return m_libId; }
     void SetLibId( const LIB_ID& aLibId ) { m_libId = aLibId; }
diff --git a/eeschema/sch_legacy_plugin.cpp b/eeschema/sch_legacy_plugin.cpp
index 968069576..0ab8e964e 100644
--- a/eeschema/sch_legacy_plugin.cpp
+++ b/eeschema/sch_legacy_plugin.cpp
@@ -2395,25 +2395,20 @@ LIB_PART* SCH_LEGACY_PLUGIN_CACHE::loadPart( FILE_LINE_READER& aReader )
         part->SetUnitCount( 1 );
 
     // Copy part name and prefix.
-    LIB_FIELD& value = part->GetValueField();
 
     // The root alias is added to the alias list by SetName() which is called by SetText().
     if( name.IsEmpty() )
     {
-        part->m_name = "~";
-        value.SetText( "~" );
+        part->SetName( "~" );
     }
     else if( name[0] != '~' )
     {
-        part->m_name = name;
-        value.SetText( name );
+        part->SetName( name );
     }
     else
     {
-        name = name.Right( name.Length() - 1 );
-        part->m_name = name;
-        value.SetText( name );
-        value.SetVisible( false );
+        part->SetName( name.Right( name.Length() - 1 ) );
+        part->GetValueField().SetVisible( false );
     }
 
     // Don't set the library alias, this is determined by the symbol library table.
@@ -2685,7 +2680,7 @@ void SCH_LEGACY_PLUGIN_CACHE::loadField( std::unique_ptr< LIB_PART >& aPart,
         // Ensure the VALUE field = the part name (can be not the case
         // with malformed libraries: edited by hand, or converted from other tools)
         if( fixedField->GetId() == VALUE )
-            fixedField->m_Text = aPart->m_name;
+            fixedField->m_Text = aPart->GetName();
     }
     else
     {
-- 
2.14.2

>From e34f33c19e362461be5be46bd29f84e163837f8b Mon Sep 17 00:00:00 2001
From: Maciej Suminski <maciej.suminski@xxxxxxx>
Date: Sun, 12 Nov 2017 20:54:04 +0100
Subject: [PATCH 2/4] Keep LIB_ID and LIB_PART name/library in sync

Updates LIB_ID::LibItemName field when a part is renamed and LIB_PART
name when a new LIB_ID is set.

Similarly, LIB_ID::LibNickName field is updated when a library set, but
there is no easy way to assign library when LIB_ID::LibNickName is
modified.
---
 eeschema/class_libentry.cpp | 31 +++++++++++++++++++++++++++----
 eeschema/class_libentry.h   |  4 ++--
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/eeschema/class_libentry.cpp b/eeschema/class_libentry.cpp
index 7cba8ac47..4b2ae65b8 100644
--- a/eeschema/class_libentry.cpp
+++ b/eeschema/class_libentry.cpp
@@ -180,7 +180,6 @@ LIB_PART::LIB_PART( const wxString& aName, PART_LIB* aLibrary ) :
     EDA_ITEM( LIB_PART_T ),
     m_me( this, null_deleter() )
 {
-    m_library             = aLibrary;
     m_dateModified        = 0;
     m_unitCount           = 1;
     m_pinNameOffset       = 40;
@@ -189,8 +188,6 @@ LIB_PART::LIB_PART( const wxString& aName, PART_LIB* aLibrary ) :
     m_showPinNumbers      = true;
     m_showPinNames        = true;
 
-    wxASSERT( !aName.IsEmpty() );
-
     // Add the MANDATORY_FIELDS in RAM only.  These are assumed to be present
     // when the field editors are invoked.
     m_drawings[LIB_FIELD_T].push_back( new LIB_FIELD( this, VALUE ) );
@@ -198,6 +195,7 @@ LIB_PART::LIB_PART( const wxString& aName, PART_LIB* aLibrary ) :
     m_drawings[LIB_FIELD_T].push_back( new LIB_FIELD( this, FOOTPRINT ) );
     m_drawings[LIB_FIELD_T].push_back( new LIB_FIELD( this, DATASHEET ) );
 
+    SetLib( aLibrary );
     SetName( aName );
 }
 
@@ -298,13 +296,29 @@ wxString LIB_PART::SubReference( int aUnit, bool aAddSeparator )
 
 void LIB_PART::SetName( const wxString& aName )
 {
-    GetValueField().SetText( aName );
+    m_libId.SetLibItemName( aName, false );
 
     // The LIB_ALIAS that is the LIB_PART name has to be created so create it.
     if( m_aliases.size() == 0 )
         m_aliases.push_back( new LIB_ALIAS( aName, this ) );
     else
         m_aliases[0]->SetName( aName );
+
+    LIB_FIELD& valueField = GetValueField();
+
+    // LIB_FIELD::SetText() calls LIB_PART::SetName(),
+    // the following if-clause is to break an infinite loop
+    if( valueField.GetText() != aName )
+        valueField.SetText( aName );
+
+}
+
+
+void LIB_PART::SetLibId( const LIB_ID& aLibId )
+{
+    m_libId.SetLibNickname( aLibId.GetLibNickname() );
+    // SetName() sets LibItemName in m_libId
+    SetName( aLibId.GetLibItemName() );
 }
 
 
@@ -1656,6 +1670,15 @@ void LIB_PART::SetConversion( bool aSetConvert )
 }
 
 
+void LIB_PART::SetLib( PART_LIB* aLibrary )
+{
+    m_library = aLibrary;
+
+    if( aLibrary )
+        m_libId.SetLibNickname( aLibrary->GetName() );
+}
+
+
 wxArrayString LIB_PART::GetAliasNames( bool aIncludeRoot ) const
 {
     wxArrayString names;
diff --git a/eeschema/class_libentry.h b/eeschema/class_libentry.h
index 1301faf1e..47a0d024b 100644
--- a/eeschema/class_libentry.h
+++ b/eeschema/class_libentry.h
@@ -282,13 +282,13 @@ public:
     const wxString& GetName() const { return m_aliases[0]->GetName(); }
 
     const LIB_ID& GetLibId() const { return m_libId; }
-    void SetLibId( const LIB_ID& aLibId ) { m_libId = aLibId; }
+    void SetLibId( const LIB_ID& aLibId );
 
     const wxString GetLibraryName();
 
     PART_LIB* GetLib()              { return m_library; }
 
-    void SetLib( PART_LIB* aLibrary ) { m_library = aLibrary; }
+    void SetLib( PART_LIB* aLibrary );
 
     wxArrayString GetAliasNames( bool aIncludeRoot = true ) const;
 
-- 
2.14.2

>From 3ba5fa94faf6e34c373e0ee90802692b33c647f7 Mon Sep 17 00:00:00 2001
From: Maciej Suminski <maciej.suminski@xxxxxxx>
Date: Sun, 12 Nov 2017 22:41:08 +0100
Subject: [PATCH 3/4] Removed LIB_PART::SetLibId()

To avoid potential incoherency, LIB_ID is defined by setting the part
name and library.
---
 eeschema/class_libentry.cpp    | 8 --------
 eeschema/class_libentry.h      | 1 -
 eeschema/project_rescue.cpp    | 1 -
 eeschema/sch_legacy_plugin.cpp | 3 ---
 4 files changed, 13 deletions(-)

diff --git a/eeschema/class_libentry.cpp b/eeschema/class_libentry.cpp
index 4b2ae65b8..8faedba79 100644
--- a/eeschema/class_libentry.cpp
+++ b/eeschema/class_libentry.cpp
@@ -314,14 +314,6 @@ void LIB_PART::SetName( const wxString& aName )
 }
 
 
-void LIB_PART::SetLibId( const LIB_ID& aLibId )
-{
-    m_libId.SetLibNickname( aLibId.GetLibNickname() );
-    // SetName() sets LibItemName in m_libId
-    SetName( aLibId.GetLibItemName() );
-}
-
-
 void LIB_PART::Draw( EDA_DRAW_PANEL* aPanel, wxDC* aDc, const wxPoint& aOffset,
             int aMulti, int aConvert, const PART_DRAW_OPTIONS& aOpts )
 {
diff --git a/eeschema/class_libentry.h b/eeschema/class_libentry.h
index 47a0d024b..08e354670 100644
--- a/eeschema/class_libentry.h
+++ b/eeschema/class_libentry.h
@@ -282,7 +282,6 @@ public:
     const wxString& GetName() const { return m_aliases[0]->GetName(); }
 
     const LIB_ID& GetLibId() const { return m_libId; }
-    void SetLibId( const LIB_ID& aLibId );
 
     const wxString GetLibraryName();
 
diff --git a/eeschema/project_rescue.cpp b/eeschema/project_rescue.cpp
index 48303d0be..0a482d1c7 100644
--- a/eeschema/project_rescue.cpp
+++ b/eeschema/project_rescue.cpp
@@ -409,7 +409,6 @@ wxString RESCUE_SYMBOL_LIB_TABLE_CANDIDATE::GetActionDescription() const
 bool RESCUE_SYMBOL_LIB_TABLE_CANDIDATE::PerformAction( RESCUER* aRescuer )
 {
     LIB_PART new_part( *m_cache_candidate );
-    new_part.SetLibId( m_new_id );
     new_part.SetName( m_new_id.GetLibItemName() );
     new_part.RemoveAllAliases();
     aRescuer->AddPart( &new_part );
diff --git a/eeschema/sch_legacy_plugin.cpp b/eeschema/sch_legacy_plugin.cpp
index 0ab8e964e..93e0a6d9c 100644
--- a/eeschema/sch_legacy_plugin.cpp
+++ b/eeschema/sch_legacy_plugin.cpp
@@ -2411,9 +2411,6 @@ LIB_PART* SCH_LEGACY_PLUGIN_CACHE::loadPart( FILE_LINE_READER& aReader )
         part->GetValueField().SetVisible( false );
     }
 
-    // Don't set the library alias, this is determined by the symbol library table.
-    part->SetLibId( LIB_ID( wxEmptyString, part->GetName() ) );
-
     // There are some code paths in SetText() that do not set the root alias to the
     // alias list so add it here if it didn't get added by SetText().
     if( !part->HasAlias( part->GetName() ) )
-- 
2.14.2

>From b87dbfc246fbd9df1fced52da53da14298d1e28d Mon Sep 17 00:00:00 2001
From: Maciej Suminski <maciej.suminski@xxxxxxx>
Date: Sun, 12 Nov 2017 23:47:27 +0100
Subject: [PATCH 4/4] Removed friendship in LIB_PART and LIB_ALIAS classes.

It is not required and may a source of unnecessary temptation.
---
 eeschema/class_libentry.h | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/eeschema/class_libentry.h b/eeschema/class_libentry.h
index 08e354670..a5095c263 100644
--- a/eeschema/class_libentry.h
+++ b/eeschema/class_libentry.h
@@ -89,8 +89,6 @@ class LIB_ALIAS : public EDA_ITEM
      */
     LIB_PART*       shared;
 
-    friend class LIB_PART;
-
 protected:
     wxString        name;
     wxString        description;    ///< documentation for info
@@ -226,10 +224,6 @@ struct PART_DRAW_OPTIONS
  */
 class LIB_PART : public EDA_ITEM
 {
-    friend class PART_LIB;
-    friend class LIB_ALIAS;
-    friend class SCH_LEGACY_PLUGIN_CACHE;
-
     PART_SPTR           m_me;               ///< http://www.boost.org/doc/libs/1_55_0/libs/smart_ptr/sp_techniques.html#weak_without_shared
     LIB_ID              m_libId;
     int                 m_pinNameOffset;    ///< The offset in mils to draw the pin name.  Set to 0
-- 
2.14.2