From a8f4dfd47d84882de9246ed33b026566e93b3eb2 Mon Sep 17 00:00:00 2001 From: John Beard Date: Tue, 17 Sep 2024 19:53:56 +0100 Subject: [PATCH] SYMBOL_BUFFER: Enforce non-null returns SYMBOL_BUFFER GetSymbol and GetOriginal cannot return null, as the SYMBOL_BUFFER class internally ensures that the original and current symbol pointers are always filled through checks in the ctor and setters. This means that clients don't need to null-check the returns, which is good because they mostly already do not. Return references instead, to formalise the non-nullity of these pointers and absolve the calling code of having to consider if they might be null. --- eeschema/symbol_library_manager.cpp | 102 +++++++++--------- eeschema/symbol_library_manager.h | 4 +- .../eeschema/test_symbol_library_manager.cpp | 10 +- 3 files changed, 55 insertions(+), 61 deletions(-) diff --git a/eeschema/symbol_library_manager.cpp b/eeschema/symbol_library_manager.cpp index 6e9385b05e..95896d100c 100644 --- a/eeschema/symbol_library_manager.cpp +++ b/eeschema/symbol_library_manager.cpp @@ -369,7 +369,7 @@ std::list SYMBOL_LIBRARY_MANAGER::GetAliases( const wxString& aLibr if( libIt != m_libs.end() ) { for( const std::shared_ptr& symbolBuf : libIt->second.GetBuffers() ) - ret.push_back( symbolBuf->GetSymbol() ); + ret.push_back( &symbolBuf->GetSymbol() ); } else { @@ -477,14 +477,12 @@ bool SYMBOL_LIBRARY_MANAGER::UpdateSymbol( LIB_SYMBOL* aSymbol, const wxString& if( symbolBuf ) // Existing symbol. { - LIB_SYMBOL* bufferedSymbol = const_cast< LIB_SYMBOL* >( symbolBuf->GetSymbol() ); - - wxCHECK( bufferedSymbol, false ); + LIB_SYMBOL& bufferedSymbol = symbolBuf->GetSymbol(); // If we are coming from a different library, the library ID needs to be preserved - auto libId = bufferedSymbol->GetLibId(); - *bufferedSymbol = *aSymbol; - bufferedSymbol->SetLibId( libId ); + const LIB_ID libId = bufferedSymbol.GetLibId(); + bufferedSymbol = *aSymbol; + bufferedSymbol.SetLibId( libId ); symbolBuf->GetScreen()->SetContentModified(); } @@ -528,7 +526,7 @@ LIB_ID SYMBOL_LIBRARY_MANAGER::RevertSymbol( const wxString& aAlias, const wxStr std::shared_ptr symbolBuf = it->second.GetBuffer( aAlias ); wxCHECK( symbolBuf, LIB_ID( aLibrary, aAlias ) ); - LIB_SYMBOL original( *symbolBuf->GetOriginal() ); + LIB_SYMBOL original( symbolBuf->GetOriginal() ); if( original.GetName() != aAlias ) { @@ -537,7 +535,7 @@ LIB_ID SYMBOL_LIBRARY_MANAGER::RevertSymbol( const wxString& aAlias, const wxStr else { // copy the initial data to the current symbol to restore - *symbolBuf->GetSymbol() = original; + symbolBuf->GetSymbol() = original; OnDataChanged(); } @@ -577,7 +575,7 @@ bool SYMBOL_LIBRARY_MANAGER::RevertAll() if( !buffer->IsModified() ) continue; - RevertSymbol( lib.first, buffer->GetOriginal()->GetName() ); + RevertSymbol( lib.first, buffer->GetOriginal().GetName() ); } } @@ -871,6 +869,7 @@ SYMBOL_BUFFER::SYMBOL_BUFFER( std::unique_ptr aSymbol, { wxASSERT( m_symbol ); m_original = std::make_unique( *m_symbol ); + wxASSERT( m_original ); } @@ -922,11 +921,8 @@ LIB_SYMBOL* LIB_BUFFER::GetSymbol( const wxString& aAlias ) const if( !buf ) return nullptr; - LIB_SYMBOL* symbol = buf->GetSymbol(); - - wxCHECK( symbol, nullptr ); - - return symbol; + LIB_SYMBOL& symbol = buf->GetSymbol(); + return &symbol; } @@ -953,11 +949,9 @@ bool LIB_BUFFER::CreateBuffer( std::unique_ptr aCopy, bool LIB_BUFFER::UpdateBuffer( SYMBOL_BUFFER& aSymbolBuf, const LIB_SYMBOL& aCopy ) { - LIB_SYMBOL* bufferedSymbol = aSymbolBuf.GetSymbol(); + LIB_SYMBOL& bufferedSymbol = aSymbolBuf.GetSymbol(); - wxCHECK( bufferedSymbol, false ); - - *bufferedSymbol = aCopy; + bufferedSymbol = aCopy; ++m_hash; return true; @@ -977,7 +971,7 @@ bool LIB_BUFFER::DeleteBuffer( const SYMBOL_BUFFER& aSymbolBuf ) bool retv = true; // Remove all derived symbols to prevent broken inheritance. - if( HasDerivedSymbols( aSymbolBuf.GetSymbol()->GetName() ) + if( HasDerivedSymbols( aSymbolBuf.GetSymbol().GetName() ) && ( removeChildSymbols( aSymbolBuf ) == 0 ) ) { retv = false; @@ -1004,25 +998,23 @@ bool LIB_BUFFER::SaveBuffer( std::shared_ptr aSymbolBuf, properties.emplace( SCH_IO_KICAD_LEGACY::PropBuffering, "" ); std::shared_ptr& symbolBuf = aSymbolBuf; - LIB_SYMBOL* libSymbol = symbolBuf->GetSymbol(); + LIB_SYMBOL& libSymbol = symbolBuf->GetSymbol(); { - LIB_SYMBOL* const originalSymbol = symbolBuf->GetOriginal(); - - wxCHECK( libSymbol && originalSymbol, false ); + LIB_SYMBOL& originalSymbol = symbolBuf->GetOriginal(); + const wxString originalName = originalSymbol.GetName(); // Delete the original symbol if the symbol name has been changed. - if( libSymbol->GetName() != originalSymbol->GetName() ) + if( libSymbol.GetName() != originalSymbol.GetName() ) { try { - if( aPlugin->LoadSymbol( aFileName, originalSymbol->GetName() ) ) - aPlugin->DeleteSymbol( aFileName, originalSymbol->GetName(), &properties ); + if( aPlugin->LoadSymbol( aFileName, originalName ) ) + aPlugin->DeleteSymbol( aFileName, originalName, &properties ); } catch( const IO_ERROR& ioe ) { - wxLogError( errorMsg, UnescapeString( originalSymbol->GetName() ), aFileName, - ioe.What() ); + wxLogError( errorMsg, UnescapeString( originalName ), aFileName, ioe.What() ); return false; } } @@ -1030,10 +1022,10 @@ bool LIB_BUFFER::SaveBuffer( std::shared_ptr aSymbolBuf, LIB_SYMBOL* parentSymbol = nullptr; - if( libSymbol->IsAlias() ) + if( libSymbol.IsAlias() ) { - LIB_SYMBOL* newCachedSymbol = new LIB_SYMBOL( *libSymbol ); - std::shared_ptr bufferedParent = libSymbol->GetParent().lock(); + LIB_SYMBOL* newCachedSymbol = new LIB_SYMBOL( libSymbol ); + std::shared_ptr bufferedParent = libSymbol.GetParent().lock(); parentSymbol = newCachedSymbol; wxCHECK( bufferedParent, false ); @@ -1080,7 +1072,7 @@ bool LIB_BUFFER::SaveBuffer( std::shared_ptr aSymbolBuf, LIB_SYMBOL& parentRef = *originalParent; aSymbolBuf->SetOriginal( std::move( originalParent ) ); - auto newSymbol = std::make_unique( *libSymbol ); + auto newSymbol = std::make_unique( libSymbol ); newSymbol->SetParent( &parentRef ); aSymbolBuf->SetOriginal( std::move( newSymbol ) ); } @@ -1102,14 +1094,14 @@ bool LIB_BUFFER::SaveBuffer( std::shared_ptr aSymbolBuf, auto originalBufferedParent = GetBuffer( bufferedParent->GetName() ); wxCHECK( originalBufferedParent, false ); - auto newSymbol = std::make_unique( *libSymbol ); - newSymbol->SetParent( originalBufferedParent->GetSymbol() ); + auto newSymbol = std::make_unique( libSymbol ); + newSymbol->SetParent( &originalBufferedParent->GetSymbol() ); aSymbolBuf->SetOriginal( std::move( newSymbol ) ); } } else { - parentSymbol = new LIB_SYMBOL( *libSymbol ); + parentSymbol = new LIB_SYMBOL( libSymbol ); try { @@ -1117,18 +1109,17 @@ bool LIB_BUFFER::SaveBuffer( std::shared_ptr aSymbolBuf, } catch( const IO_ERROR& ioe ) { - wxLogError( errorMsg, UnescapeString( libSymbol->GetName() ), aFileName, - ioe.What() ); + wxLogError( errorMsg, UnescapeString( libSymbol.GetName() ), aFileName, ioe.What() ); return false; } - aSymbolBuf->SetOriginal( std::make_unique( *libSymbol ) ); + aSymbolBuf->SetOriginal( std::make_unique( libSymbol ) ); } wxArrayString derivedSymbols; // Reparent all symbols derived from the saved symbol. - if( GetDerivedSymbolNames( libSymbol->GetName(), derivedSymbols ) != 0 ) + if( GetDerivedSymbolNames( libSymbol.GetName(), derivedSymbols ) != 0 ) { // Save the derived symbols. for( const wxString& entry : derivedSymbols ) @@ -1137,7 +1128,7 @@ bool LIB_BUFFER::SaveBuffer( std::shared_ptr aSymbolBuf, wxCHECK2( symbol, continue ); - LIB_SYMBOL* derivedSymbol = new LIB_SYMBOL( *symbol->GetSymbol() ); + LIB_SYMBOL* derivedSymbol = new LIB_SYMBOL( symbol->GetSymbol() ); derivedSymbol->SetParent( parentSymbol ); try @@ -1163,7 +1154,7 @@ std::shared_ptr LIB_BUFFER::GetBuffer( const wxString& aAlias ) c { for( std::shared_ptr entry : m_symbols ) { - if( entry->GetSymbol()->GetName() == aAlias ) + if( entry->GetSymbol().GetName() == aAlias ) return entry; } @@ -1175,9 +1166,9 @@ bool LIB_BUFFER::HasDerivedSymbols( const wxString& aParentName ) const { for( const std::shared_ptr& entry : m_symbols ) { - if( entry->GetSymbol()->IsAlias() ) + if( entry->GetSymbol().IsAlias() ) { - LIB_SYMBOL_SPTR parent = entry->GetSymbol()->GetParent().lock(); + LIB_SYMBOL_SPTR parent = entry->GetSymbol().GetParent().lock(); // Check for inherited symbol without a valid parent. wxCHECK( parent, false ); @@ -1195,11 +1186,13 @@ void LIB_BUFFER::GetSymbolNames( wxArrayString& aSymbolNames, SYMBOL_NAME_FILTER { for( std::shared_ptr& entry : m_symbols ) { - if( ( entry->GetSymbol()->IsAlias() && ( aFilter == SYMBOL_NAME_FILTER::ROOT_ONLY ) ) - || ( entry->GetSymbol()->IsRoot() && ( aFilter == SYMBOL_NAME_FILTER::DERIVED_ONLY ) ) ) + const LIB_SYMBOL& symbol = entry->GetSymbol(); + if( ( symbol.IsAlias() && ( aFilter == SYMBOL_NAME_FILTER::ROOT_ONLY ) ) + || ( symbol.IsRoot() && ( aFilter == SYMBOL_NAME_FILTER::DERIVED_ONLY ) ) ) + { continue; - - aSymbolNames.Add( UnescapeString( entry->GetSymbol()->GetName() ) ); + } + aSymbolNames.Add( UnescapeString( symbol.GetName() ) ); } } @@ -1210,18 +1203,19 @@ size_t LIB_BUFFER::GetDerivedSymbolNames( const wxString& aSymbolName, wxArraySt for( std::shared_ptr& entry : m_symbols ) { - if( entry->GetSymbol()->IsAlias() ) + const LIB_SYMBOL& symbol = entry->GetSymbol(); + if( symbol.IsAlias() ) { - LIB_SYMBOL_SPTR parent = entry->GetSymbol()->GetParent().lock(); + LIB_SYMBOL_SPTR parent = symbol.GetParent().lock(); // Check for inherited symbol without a valid parent. wxCHECK2( parent, continue ); if( parent->GetName() == aSymbolName ) { - aList.Add( entry->GetSymbol()->GetName() ); + aList.Add( symbol.GetName() ); - GetDerivedSymbolNames( entry->GetSymbol()->GetName(), aList ); + GetDerivedSymbolNames( symbol.GetName(), aList ); } } } @@ -1236,14 +1230,14 @@ int LIB_BUFFER::removeChildSymbols( const SYMBOL_BUFFER& aSymbolBuf ) wxArrayString derivedSymbolNames; std::deque>::iterator it; - if( GetDerivedSymbolNames( aSymbolBuf.GetSymbol()->GetName(), derivedSymbolNames ) ) + if( GetDerivedSymbolNames( aSymbolBuf.GetSymbol().GetName(), derivedSymbolNames ) ) { for( const wxString& symbolName : derivedSymbolNames ) { it = std::find_if( m_symbols.begin(), m_symbols.end(), [symbolName]( std::shared_ptr& buf ) { - return buf->GetSymbol()->GetName() == symbolName; + return buf->GetSymbol().GetName() == symbolName; } ); wxCHECK2( it != m_symbols.end(), continue ); diff --git a/eeschema/symbol_library_manager.h b/eeschema/symbol_library_manager.h index 639eb1a767..9cd6250953 100644 --- a/eeschema/symbol_library_manager.h +++ b/eeschema/symbol_library_manager.h @@ -61,10 +61,10 @@ public: std::unique_ptr aScreen = nullptr ); ~SYMBOL_BUFFER(); - LIB_SYMBOL* GetSymbol() const { return m_symbol.get(); } + LIB_SYMBOL& GetSymbol() const { return *m_symbol; } void SetSymbol( std::unique_ptr aSymbol ); - LIB_SYMBOL* GetOriginal() const { return m_original.get(); } + LIB_SYMBOL& GetOriginal() const { return *m_original; } void SetOriginal( std::unique_ptr aSymbol ); bool IsModified() const; diff --git a/qa/tests/eeschema/test_symbol_library_manager.cpp b/qa/tests/eeschema/test_symbol_library_manager.cpp index c2754c25aa..12477e6e9b 100644 --- a/qa/tests/eeschema/test_symbol_library_manager.cpp +++ b/qa/tests/eeschema/test_symbol_library_manager.cpp @@ -53,8 +53,8 @@ BOOST_AUTO_TEST_CASE( SymbolBuffer ) SYMBOL_BUFFER buffer( std::move( symbol ), std::move( screen ) ); BOOST_CHECK( !buffer.IsModified() ); - BOOST_CHECK( buffer.GetSymbol() == &symbolRef ); - BOOST_CHECK( *buffer.GetOriginal() == symbolRef ); + BOOST_CHECK( &buffer.GetSymbol() == &symbolRef ); + BOOST_CHECK( buffer.GetOriginal() == symbolRef ); buffer.GetScreen()->SetContentModified(); BOOST_CHECK( buffer.IsModified() ); @@ -65,9 +65,9 @@ BOOST_AUTO_TEST_CASE( SymbolBuffer ) buffer.SetOriginal( std::move( originalSymbol ) ); - BOOST_CHECK( buffer.GetOriginal() == &originalSymbolRef ); - BOOST_CHECK( *buffer.GetOriginal() == originalSymbolRef ); - BOOST_CHECK( *buffer.GetSymbol() != *buffer.GetOriginal() ); + BOOST_CHECK( &buffer.GetOriginal() == &originalSymbolRef ); + BOOST_CHECK( buffer.GetOriginal() == originalSymbolRef ); + BOOST_CHECK( &buffer.GetSymbol() != &buffer.GetOriginal() ); }