From 228edd412143a5cb7ae8612ba1759205d84e6770 Mon Sep 17 00:00:00 2001 From: Jeff Young Date: Wed, 24 Nov 2021 11:02:38 +0000 Subject: [PATCH] Don't generate duplicate IDs in line/wire/bus tool. Also cleans up existing duplicate IDs when reading. Fixes https://gitlab.com/kicad/code/kicad/issues/9749 --- common/kiid.cpp | 33 ++++++++++++----- .../sch_plugins/kicad/sch_sexpr_parser.cpp | 35 +++++++++++++------ eeschema/sch_plugins/kicad/sch_sexpr_parser.h | 4 +++ eeschema/tools/sch_line_wire_bus_tool.cpp | 4 +-- include/kiid.h | 16 +++++++++ 5 files changed, 71 insertions(+), 21 deletions(-) diff --git a/common/kiid.cpp b/common/kiid.cpp index 16806551bd..d37fbd1c46 100644 --- a/common/kiid.cpp +++ b/common/kiid.cpp @@ -38,21 +38,23 @@ #include // boost:mt19937 is not thread-safe -static std::mutex rng_mutex; +static std::mutex rng_mutex; // Create only once, as seeding is *very* expensive -static boost::mt19937 rng; +static boost::mt19937 rng; static boost::uuids::basic_random_generator randomGenerator( rng ); -// These don't have the same performance penalty, but might as well be consistent -static boost::uuids::string_generator stringGenerator; -static boost::uuids::nil_generator nilGenerator; +// These don't have the same performance penalty, but we might as well be consistent +static boost::uuids::string_generator stringGenerator; +static boost::uuids::nil_generator nilGenerator; + // Global nil reference KIID niluuid( 0 ); + // When true, always create nil uuids for performance, when valid ones aren't needed -static bool createNilUuids = false; +static bool g_createNilUuids = false; // For static initialization @@ -72,7 +74,7 @@ KIID::KIID() { #endif - if( createNilUuids ) + if( g_createNilUuids ) { m_uuid = nilGenerator(); } @@ -247,9 +249,24 @@ void KIID::ConvertTimestampToUuid() } +void KIID::Increment() +{ + // This obviously destroys uniform distribution, but it can be useful when a + // deterministic replacement for a duplicate ID is required. + + for( int i = 15; i >= 0; --i ) + { + m_uuid.data[i]++; + + if( m_uuid.data[i] != 0 ) + break; + } +} + + void KIID::CreateNilUuids( bool aNil ) { - createNilUuids = aNil; + g_createNilUuids = aNil; } diff --git a/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp b/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp index 0bc9d8d437..9221a9f97e 100644 --- a/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp +++ b/eeschema/sch_plugins/kicad/sch_sexpr_parser.cpp @@ -94,6 +94,19 @@ void SCH_SEXPR_PARSER::checkpoint() } +KIID SCH_SEXPR_PARSER::parseKIID() +{ + KIID id( FromUTF8() ); + + while( m_uuids.count( id ) ) + id.Increment(); + + m_uuids.insert( id ); + + return id; +} + + bool SCH_SEXPR_PARSER::parseBool() { T token = NextTok(); @@ -1838,7 +1851,7 @@ SCH_SHEET_PIN* SCH_SEXPR_PARSER::parseSchSheetPin( SCH_SHEET* aSheet ) case T_uuid: NeedSYMBOL(); - const_cast( sheetPin->m_Uuid ) = KIID( FromUTF8() ); + const_cast( sheetPin->m_Uuid ) = parseKIID(); NeedRIGHT(); break; @@ -2050,7 +2063,7 @@ void SCH_SEXPR_PARSER::ParseSchematic( SCH_SHEET* aSheet, bool aIsCopyableOnly, { case T_uuid: NeedSYMBOL(); - screen->m_uuid = KIID( FromUTF8() ); + screen->m_uuid = parseKIID(); NeedRIGHT(); break; @@ -2308,7 +2321,7 @@ SCH_SYMBOL* SCH_SEXPR_PARSER::parseSchematicSymbol() case T_uuid: NeedSYMBOL(); - const_cast( symbol->m_Uuid ) = KIID( FromUTF8() ); + const_cast( symbol->m_Uuid ) = parseKIID(); NeedRIGHT(); break; @@ -2395,7 +2408,7 @@ SCH_SYMBOL* SCH_SEXPR_PARSER::parseSchematicSymbol() // First version to write out pin uuids accidentally wrote out the symbol's // uuid for each pin, so ignore uuids coming from that version. if( m_requiredVersion >= 20210126 ) - uuid = KIID( FromUTF8() ); + uuid = parseKIID(); NeedRIGHT(); break; @@ -2460,7 +2473,7 @@ SCH_BITMAP* SCH_SEXPR_PARSER::parseImage() case T_uuid: NeedSYMBOL(); - const_cast( bitmap->m_Uuid ) = KIID( FromUTF8() ); + const_cast( bitmap->m_Uuid ) = parseKIID(); NeedRIGHT(); break; @@ -2560,7 +2573,7 @@ SCH_SHEET* SCH_SEXPR_PARSER::parseSheet() case T_uuid: NeedSYMBOL(); - const_cast( sheet->m_Uuid ) = KIID( FromUTF8() ); + const_cast( sheet->m_Uuid ) = parseKIID(); NeedRIGHT(); break; @@ -2652,7 +2665,7 @@ SCH_JUNCTION* SCH_SEXPR_PARSER::parseJunction() case T_uuid: NeedSYMBOL(); - const_cast( junction->m_Uuid ) = KIID( FromUTF8() ); + const_cast( junction->m_Uuid ) = parseKIID(); NeedRIGHT(); break; @@ -2689,7 +2702,7 @@ SCH_NO_CONNECT* SCH_SEXPR_PARSER::parseNoConnect() case T_uuid: NeedSYMBOL(); - const_cast( no_connect->m_Uuid ) = KIID( FromUTF8() ); + const_cast( no_connect->m_Uuid ) = parseKIID(); NeedRIGHT(); break; @@ -2743,7 +2756,7 @@ SCH_BUS_WIRE_ENTRY* SCH_SEXPR_PARSER::parseBusEntry() case T_uuid: NeedSYMBOL(); - const_cast( busEntry->m_Uuid ) = KIID( FromUTF8() ); + const_cast( busEntry->m_Uuid ) = parseKIID(); NeedRIGHT(); break; @@ -2808,7 +2821,7 @@ SCH_LINE* SCH_SEXPR_PARSER::parseLine() case T_uuid: NeedSYMBOL(); - const_cast( line->m_Uuid ) = KIID( FromUTF8() ); + const_cast( line->m_Uuid ) = parseKIID(); NeedRIGHT(); break; @@ -2934,7 +2947,7 @@ SCH_TEXT* SCH_SEXPR_PARSER::parseSchText() case T_uuid: NeedSYMBOL(); - const_cast( text->m_Uuid ) = KIID( FromUTF8() ); + const_cast( text->m_Uuid ) = parseKIID(); NeedRIGHT(); break; diff --git a/eeschema/sch_plugins/kicad/sch_sexpr_parser.h b/eeschema/sch_plugins/kicad/sch_sexpr_parser.h index 5e0ae6fe17..0cd7239430 100644 --- a/eeschema/sch_plugins/kicad/sch_sexpr_parser.h +++ b/eeschema/sch_plugins/kicad/sch_sexpr_parser.h @@ -81,6 +81,8 @@ class SCH_SEXPR_PARSER : public SCHEMATIC_LEXER int m_convert; ///< The current body style being parsed. wxString m_symbolName; ///< The current symbol name. + std::set m_uuids; + PROGRESS_REPORTER* m_progressReporter; // optional; may be nullptr const LINE_READER* m_lineReader; // for progress reporting unsigned m_lastProgressLine; @@ -88,6 +90,8 @@ class SCH_SEXPR_PARSER : public SCHEMATIC_LEXER void checkpoint(); + KIID parseKIID(); + void parseHeader( TSCHEMATIC_T::T aHeaderType, int aFileVersion ); inline long parseHex() diff --git a/eeschema/tools/sch_line_wire_bus_tool.cpp b/eeschema/tools/sch_line_wire_bus_tool.cpp index dd825831c7..ed818a10cc 100644 --- a/eeschema/tools/sch_line_wire_bus_tool.cpp +++ b/eeschema/tools/sch_line_wire_bus_tool.cpp @@ -651,7 +651,7 @@ int SCH_LINE_WIRE_BUS_TOOL::doDrawSegments( const std::string& aTool, int aType, segment->SetEndPoint( cursorPos ); // Create a new segment, and chain it after the current segment. - segment = new SCH_LINE( *segment ); + segment = static_cast( segment->Duplicate() ); segment->SetFlags( IS_NEW | IS_MOVING ); segment->SetStartPoint( cursorPos ); m_wires.push_back( segment ); @@ -813,7 +813,7 @@ SCH_LINE* SCH_LINE_WIRE_BUS_TOOL::startSegments( int aType, const VECTOR2D& aPos // horizontal and vertical lines only switch is on. if( m_frame->eeconfig()->m_Drawing.hv_lines_only ) { - segment = new SCH_LINE( *segment ); + segment = static_cast( segment->Duplicate() ); segment->SetFlags( IS_NEW | IS_MOVING ); m_wires.push_back( segment ); diff --git a/include/kiid.h b/include/kiid.h index d2a3942150..57e8484df8 100644 --- a/include/kiid.h +++ b/include/kiid.h @@ -59,8 +59,16 @@ public: wxString AsString() const; wxString AsLegacyTimestampString() const; + /** + * Returns true if a string has the correct formatting to be a KIID. + */ static bool SniffTest( const wxString& aCandidate ); + /** + * A performance optimization which disables/enables the generation of pseudo-random UUIDs. + * + * NB: uses a global. Not thread safe! + */ static void CreateNilUuids( bool aNil = true ); /** @@ -81,6 +89,14 @@ public: */ void ConvertTimestampToUuid(); + /** + * Generates a deterministic replacement for a given ID. + * + * NB: destroys uniform distribution! But it's the only thing we have when a deterministic + * replacement for a duplicate ID is required. + */ + void Increment(); + bool operator==( KIID const& rhs ) const { return m_uuid == rhs.m_uuid;