From 8a305eec322032f9bf538ee652ae2395edc1f26e Mon Sep 17 00:00:00 2001 From: Seth Hillbrand Date: Wed, 1 Sep 2021 11:45:10 -0700 Subject: [PATCH] Remove broken RefDesStringCompare This function attempted a poor-man's natural compare but it assumed specific structure of the string. This broke for strings with numberings that looked like decimals. Instead, we use our natural string comparison function and remove the references to this older function and its similar elements. Fixes https://gitlab.com/kicad/code/kicad/issues/9067 --- common/refdes_utils.cpp | 44 +------------------ .../dialogs/dialog_symbol_fields_table.cpp | 13 +++--- .../netlist_exporter_base.cpp | 4 +- .../netlist_exporter_xml.cpp | 8 ++-- eeschema/sch_reference_list.h | 2 +- include/refdes_utils.h | 11 +---- pcbnew/netlist_reader/pcb_netlist.cpp | 4 +- pcbnew/tools/board_reannotate_tool.cpp | 3 +- qa/common/test_kicad_string.cpp | 8 +++- qa/common/test_refdes_utils.cpp | 32 -------------- 10 files changed, 26 insertions(+), 103 deletions(-) diff --git a/common/refdes_utils.cpp b/common/refdes_utils.cpp index 983b034307..3b15518400 100644 --- a/common/refdes_utils.cpp +++ b/common/refdes_utils.cpp @@ -70,46 +70,4 @@ int GetRefDesNumber( const wxString& aRefDes ) return retval; } - -int RefDesStringCompare( const wxString& aFirst, const wxString& aSecond ) -{ - // Compare unescaped text - wxString strFWord = UnescapeString( aFirst ); - wxString strSWord = UnescapeString( aSecond ); - - // The different sections of the two strings - wxString strFWordBeg, strFWordMid, strFWordEnd; - wxString strSWordBeg, strSWordMid, strSWordEnd; - - // Split the two strings into separate parts - SplitString( strFWord, &strFWordBeg, &strFWordMid, &strFWordEnd ); - SplitString( strSWord, &strSWordBeg, &strSWordMid, &strSWordEnd ); - - // Compare the Beginning section of the strings - int isEqual = strFWordBeg.Cmp( strSWordBeg ); - - if( isEqual > 0 ) - return 1; - else if( isEqual < 0 ) - return -1; - else - { - // If the first sections are equal compare their digits - long lFirstDigit = 0; - long lSecondDigit = 0; - - strFWordMid.ToLong( &lFirstDigit ); - strSWordMid.ToLong( &lSecondDigit ); - - if( lFirstDigit > lSecondDigit ) - return 1; - else if( lFirstDigit < lSecondDigit ) - return -1; - // If the first two sections are equal compare the endings - else - return strFWordEnd.Cmp( strSWordEnd ); - } -} - - -} // namespace UTIL \ No newline at end of file +} // namespace UTIL diff --git a/eeschema/dialogs/dialog_symbol_fields_table.cpp b/eeschema/dialogs/dialog_symbol_fields_table.cpp index 08626dbbe0..69bbbe0386 100644 --- a/eeschema/dialogs/dialog_symbol_fields_table.cpp +++ b/eeschema/dialogs/dialog_symbol_fields_table.cpp @@ -33,7 +33,6 @@ #include #include #include -#include #include #include #include @@ -302,9 +301,9 @@ public: std::sort( references.begin(), references.end(), []( const SCH_REFERENCE& l, const SCH_REFERENCE& r ) -> bool { - wxString l_ref( l.GetRef() << l.GetRefNumber() ); - wxString r_ref( r.GetRef() << r.GetRefNumber() ); - return UTIL::RefDesStringCompare( l_ref, r_ref ) < 0; + wxString l_ref( l.GetFullRef() ); + wxString r_ref( r.GetFullRef() ); + return StrNumCmp( l_ref, r_ref, true ) < 0; } ); auto logicalEnd = std::unique( references.begin(), references.end(), @@ -372,9 +371,9 @@ public: if( lhs == rhs || sortCol == REFERENCE_FIELD ) { - wxString lhRef = lhGroup.m_Refs[ 0 ].GetRef() + lhGroup.m_Refs[ 0 ].GetRefNumber(); - wxString rhRef = rhGroup.m_Refs[ 0 ].GetRef() + rhGroup.m_Refs[ 0 ].GetRefNumber(); - return local_cmp( UTIL::RefDesStringCompare( lhRef, rhRef ), 0 ); + wxString lhRef = lhGroup.m_Refs[ 0 ].GetFullRef(); + wxString rhRef = rhGroup.m_Refs[ 0 ].GetFullRef(); + return local_cmp( StrNumCmp( lhRef, rhRef, true ), 0 ); } else { diff --git a/eeschema/netlist_exporters/netlist_exporter_base.cpp b/eeschema/netlist_exporters/netlist_exporter_base.cpp index d683bab9e8..7050d4b7be 100644 --- a/eeschema/netlist_exporters/netlist_exporter_base.cpp +++ b/eeschema/netlist_exporters/netlist_exporter_base.cpp @@ -25,7 +25,7 @@ #include -#include +#include #include #include @@ -117,7 +117,7 @@ SCH_SYMBOL* NETLIST_EXPORTER_BASE::findNextSymbol( EDA_ITEM* aItem, SCH_SHEET_PA static bool sortPinsByNum( PIN_INFO& aPin1, PIN_INFO& aPin2 ) { // return "lhs < rhs" - return UTIL::RefDesStringCompare( aPin1.num, aPin2.num ) < 0; + return StrNumCmp( aPin1.num, aPin2.num, true ) < 0; } diff --git a/eeschema/netlist_exporters/netlist_exporter_xml.cpp b/eeschema/netlist_exporters/netlist_exporter_xml.cpp index 90e82fadea..a653567e1e 100644 --- a/eeschema/netlist_exporters/netlist_exporter_xml.cpp +++ b/eeschema/netlist_exporters/netlist_exporter_xml.cpp @@ -31,7 +31,7 @@ #include #include #include -#include +#include #include #include // also nests: @@ -237,8 +237,8 @@ XNODE* NETLIST_EXPORTER_XML::makeSymbols( unsigned aCtl ) auto cmp = [sheet]( SCH_SYMBOL* a, SCH_SYMBOL* b ) { - return ( UTIL::RefDesStringCompare( a->GetRef( &sheet ), - b->GetRef( &sheet ) ) < 0 ); + return ( StrNumCmp( a->GetRef( &sheet, false ), + b->GetRef( &sheet, false ), true ) < 0 ); }; std::set ordered_symbols( cmp ); @@ -779,5 +779,5 @@ XNODE* NETLIST_EXPORTER_XML::node( const wxString& aName, static bool sortPinsByNumber( LIB_PIN* aPin1, LIB_PIN* aPin2 ) { // return "lhs < rhs" - return UTIL::RefDesStringCompare( aPin1->GetShownNumber(), aPin2->GetShownNumber() ) < 0; + return StrNumCmp( aPin1->GetShownNumber(), aPin2->GetShownNumber(), true ) < 0; } diff --git a/eeschema/sch_reference_list.h b/eeschema/sch_reference_list.h index ef2a849a31..407743a172 100644 --- a/eeschema/sch_reference_list.h +++ b/eeschema/sch_reference_list.h @@ -104,7 +104,7 @@ public: const char* GetRefStr() const { return m_ref.c_str(); } ///< Return reference name with unit altogether - wxString GetFullRef() + wxString GetFullRef() const { if( GetSymbol()->GetUnitCount() > 1 ) return GetRef() + LIB_SYMBOL::SubReference( GetUnit() ); diff --git a/include/refdes_utils.h b/include/refdes_utils.h index bf244c872d..f86dde3440 100644 --- a/include/refdes_utils.h +++ b/include/refdes_utils.h @@ -64,15 +64,6 @@ wxString GetRefDesUnannotated( const wxString& aRefDes ); */ int GetRefDesNumber( const wxString& aRefDes ); -/** - * Acts just like the strcmp function but treats numbers within the string text - * correctly for sorting. eg. A10 > A2 - * return -1 if first string is less than the second - * return 0 if the strings are equal - * return 1 if the first string is greater than the second - */ -int RefDesStringCompare( const wxString& lhs, const wxString& rhs ); - } // namespace UTIL -#endif // REFDES_UTILS__H \ No newline at end of file +#endif // REFDES_UTILS__H diff --git a/pcbnew/netlist_reader/pcb_netlist.cpp b/pcbnew/netlist_reader/pcb_netlist.cpp index 4ac89a39de..e16947b279 100644 --- a/pcbnew/netlist_reader/pcb_netlist.cpp +++ b/pcbnew/netlist_reader/pcb_netlist.cpp @@ -24,7 +24,7 @@ */ -#include +#include #include "pcb_netlist.h" #include @@ -211,7 +211,7 @@ void NETLIST::SortByFPID() */ bool operator < ( const COMPONENT& item1, const COMPONENT& item2 ) { - return UTIL::RefDesStringCompare( item1.GetReference(), item2.GetReference() ) < 0; + return StrNumCmp( item1.GetReference(), item2.GetReference(), true ) < 0; } diff --git a/pcbnew/tools/board_reannotate_tool.cpp b/pcbnew/tools/board_reannotate_tool.cpp index 8d8617d6dd..f6d760c995 100644 --- a/pcbnew/tools/board_reannotate_tool.cpp +++ b/pcbnew/tools/board_reannotate_tool.cpp @@ -24,6 +24,7 @@ */ #include +#include #include #include #include @@ -86,7 +87,7 @@ int BOARD_REANNOTATE_TOOL::ReannotateDuplicatesInSelection() std::sort( fpInSelection.begin(), fpInSelection.end(), []( const FOOTPRINT* aA, const FOOTPRINT* aB ) -> bool { - int ii = UTIL::RefDesStringCompare( aA->GetReference(), aB->GetReference() ); + int ii = StrNumCmp( aA->GetReference(), aB->GetReference(), true ); if( ii == 0 ) ii = aA->m_Uuid < aB->m_Uuid; // ensure a deterministic sort diff --git a/qa/common/test_kicad_string.cpp b/qa/common/test_kicad_string.cpp index 73d913148b..37b4e4bcf4 100644 --- a/qa/common/test_kicad_string.cpp +++ b/qa/common/test_kicad_string.cpp @@ -93,7 +93,13 @@ BOOST_AUTO_TEST_CASE( NaturalNumberCompare ) { { "u10", "u10" }, { 0, 0 } }, { { "U10", "U10" }, { 0, 0 } }, { { "u10", "U10" }, { 1, 0 } }, - { { "U10", "u10" }, { -1, 0 } } + { { "U10", "u10" }, { -1, 0 } }, + { { "U10.1", "U10.10" }, { -1, -1 } }, + { { "U10-1", "U10-10" }, { -1, -1 } }, + { { "U10,1", "U10,10" }, { -1, -1 } }, + { { "U10.A", "U10.a" }, { -1, 0 } }, + { { "U10-A", "U10-a" }, { -1, 0 } }, + { { "U10,A", "U10,a" }, { -1, 0 } }, }; for( const auto& c : cases ) diff --git a/qa/common/test_refdes_utils.cpp b/qa/common/test_refdes_utils.cpp index d140e153af..47d827f20c 100644 --- a/qa/common/test_refdes_utils.cpp +++ b/qa/common/test_refdes_utils.cpp @@ -66,36 +66,4 @@ BOOST_AUTO_TEST_CASE( GetPrefix ) } -struct REF_DES_COMP_CASE -{ - std::string m_refdes_a; - std::string m_refdes_b; - int m_exp_res; -}; - -/** - * Test the #UTIL::RefDesStringCompare function - */ -BOOST_AUTO_TEST_CASE( RefDesComp ) -{ - const int SAME = 0; - const int LESS = -1; - const int MORE = 1; - - const std::vector cases = { - { "", "", SAME }, - { "U", "U", SAME }, - { "U1", "U1", SAME }, - { "U1", "U2", LESS }, - { "U2", "U1", MORE }, - { "U1000", "U2000", LESS }, - }; - - for( const auto& c : cases ) - { - BOOST_CHECK_EQUAL( UTIL::RefDesStringCompare( c.m_refdes_a, c.m_refdes_b ), c.m_exp_res ); - } -} - - BOOST_AUTO_TEST_SUITE_END()