realAscot/kicad-source
1
0
Fork 0
mirror of https://gitlab.com/kicad/code/kicad.git synced 2025-09-14 10:13:19 +02:00
Code Issues Packages Projects Releases Wiki Activity

SYMBOL_BUFFER: Enforce non-null returns

Browse Source 
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.
This commit is contained in:
John Beard 2024-09-17 19:53:56 +01:00
parent 06eab304e5
commit a8f4dfd47d
3 changed files with 55 additions and 61 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

102
eeschema/symbol_library_manager.cpp
View File

@ -369,7 +369,7 @@ std::list<LIB_SYMBOL*> SYMBOL_LIBRARY_MANAGER::GetAliases( const wxString& aLibr
if( libIt != m_libs.end() ) if( libIt != m_libs.end() )
{ {
for( const std::shared_ptr<SYMBOL_BUFFER>& symbolBuf : libIt->second.GetBuffers() ) for( const std::shared_ptr<SYMBOL_BUFFER>& symbolBuf : libIt->second.GetBuffers() )
ret.push_back( symbolBuf->GetSymbol() ); ret.push_back( &symbolBuf->GetSymbol() );
} }
else else
{ {
@ -477,14 +477,12 @@ bool SYMBOL_LIBRARY_MANAGER::UpdateSymbol( LIB_SYMBOL* aSymbol, const wxString&
if( symbolBuf ) // Existing symbol. if( symbolBuf ) // Existing symbol.
{ {
LIB_SYMBOL* bufferedSymbol = const_cast< LIB_SYMBOL* >( symbolBuf->GetSymbol() ); LIB_SYMBOL& bufferedSymbol = symbolBuf->GetSymbol();
wxCHECK( bufferedSymbol, false );
// If we are coming from a different library, the library ID needs to be preserved // If we are coming from a different library, the library ID needs to be preserved
auto libId = bufferedSymbol->GetLibId(); const LIB_ID libId = bufferedSymbol.GetLibId();
*bufferedSymbol = *aSymbol; bufferedSymbol = *aSymbol;
bufferedSymbol->SetLibId( libId ); bufferedSymbol.SetLibId( libId );
symbolBuf->GetScreen()->SetContentModified(); symbolBuf->GetScreen()->SetContentModified();
} }
@ -528,7 +526,7 @@ LIB_ID SYMBOL_LIBRARY_MANAGER::RevertSymbol( const wxString& aAlias, const wxStr
std::shared_ptr<SYMBOL_BUFFER> symbolBuf = it->second.GetBuffer( aAlias ); std::shared_ptr<SYMBOL_BUFFER> symbolBuf = it->second.GetBuffer( aAlias );
wxCHECK( symbolBuf, LIB_ID( aLibrary, aAlias ) ); wxCHECK( symbolBuf, LIB_ID( aLibrary, aAlias ) );
LIB_SYMBOL original( *symbolBuf->GetOriginal() ); LIB_SYMBOL original( symbolBuf->GetOriginal() );
if( original.GetName() != aAlias ) if( original.GetName() != aAlias )
{ {
@ -537,7 +535,7 @@ LIB_ID SYMBOL_LIBRARY_MANAGER::RevertSymbol( const wxString& aAlias, const wxStr
else else
{ {
// copy the initial data to the current symbol to restore // copy the initial data to the current symbol to restore
*symbolBuf->GetSymbol() = original; symbolBuf->GetSymbol() = original;
OnDataChanged(); OnDataChanged();
} }
@ -577,7 +575,7 @@ bool SYMBOL_LIBRARY_MANAGER::RevertAll()
if( !buffer->IsModified() ) if( !buffer->IsModified() )
continue; continue;
RevertSymbol( lib.first, buffer->GetOriginal()->GetName() ); RevertSymbol( lib.first, buffer->GetOriginal().GetName() );
} }
} }
@ -871,6 +869,7 @@ SYMBOL_BUFFER::SYMBOL_BUFFER( std::unique_ptr<LIB_SYMBOL> aSymbol,
{ {
wxASSERT( m_symbol ); wxASSERT( m_symbol );
m_original = std::make_unique<LIB_SYMBOL>( *m_symbol ); m_original = std::make_unique<LIB_SYMBOL>( *m_symbol );
wxASSERT( m_original );
} }
@ -922,11 +921,8 @@ LIB_SYMBOL* LIB_BUFFER::GetSymbol( const wxString& aAlias ) const
if( !buf ) if( !buf )
return nullptr; return nullptr;
LIB_SYMBOL* symbol = buf->GetSymbol(); LIB_SYMBOL& symbol = buf->GetSymbol();
return &symbol;
wxCHECK( symbol, nullptr );
return symbol;
} }
@ -953,11 +949,9 @@ bool LIB_BUFFER::CreateBuffer( std::unique_ptr<LIB_SYMBOL> aCopy,
bool LIB_BUFFER::UpdateBuffer( SYMBOL_BUFFER& aSymbolBuf, const LIB_SYMBOL& 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; ++m_hash;
return true; return true;
@ -977,7 +971,7 @@ bool LIB_BUFFER::DeleteBuffer( const SYMBOL_BUFFER& aSymbolBuf )
bool retv = true; bool retv = true;
// Remove all derived symbols to prevent broken inheritance. // Remove all derived symbols to prevent broken inheritance.
if( HasDerivedSymbols( aSymbolBuf.GetSymbol()->GetName() ) if( HasDerivedSymbols( aSymbolBuf.GetSymbol().GetName() )
&& ( removeChildSymbols( aSymbolBuf ) == 0 ) ) && ( removeChildSymbols( aSymbolBuf ) == 0 ) )
{ {
retv = false; retv = false;
@ -1004,25 +998,23 @@ bool LIB_BUFFER::SaveBuffer( std::shared_ptr<SYMBOL_BUFFER> aSymbolBuf,
properties.emplace( SCH_IO_KICAD_LEGACY::PropBuffering, "" ); properties.emplace( SCH_IO_KICAD_LEGACY::PropBuffering, "" );
std::shared_ptr<SYMBOL_BUFFER>& symbolBuf = aSymbolBuf; std::shared_ptr<SYMBOL_BUFFER>& symbolBuf = aSymbolBuf;
LIB_SYMBOL* libSymbol = symbolBuf->GetSymbol(); LIB_SYMBOL& libSymbol = symbolBuf->GetSymbol();
{ {
LIB_SYMBOL* const originalSymbol = symbolBuf->GetOriginal(); LIB_SYMBOL& originalSymbol = symbolBuf->GetOriginal();
const wxString originalName = originalSymbol.GetName();
wxCHECK( libSymbol && originalSymbol, false );
// Delete the original symbol if the symbol name has been changed. // Delete the original symbol if the symbol name has been changed.
if( libSymbol->GetName() != originalSymbol->GetName() ) if( libSymbol.GetName() != originalSymbol.GetName() )
{ {
try try
{ {
if( aPlugin->LoadSymbol( aFileName, originalSymbol->GetName() ) ) if( aPlugin->LoadSymbol( aFileName, originalName ) )
aPlugin->DeleteSymbol( aFileName, originalSymbol->GetName(), &properties ); aPlugin->DeleteSymbol( aFileName, originalName, &properties );
} }
catch( const IO_ERROR& ioe ) catch( const IO_ERROR& ioe )
{ {
wxLogError( errorMsg, UnescapeString( originalSymbol->GetName() ), aFileName, wxLogError( errorMsg, UnescapeString( originalName ), aFileName, ioe.What() );
ioe.What() );
return false; return false;
} }
} }
@ -1030,10 +1022,10 @@ bool LIB_BUFFER::SaveBuffer( std::shared_ptr<SYMBOL_BUFFER> aSymbolBuf,
LIB_SYMBOL* parentSymbol = nullptr; LIB_SYMBOL* parentSymbol = nullptr;
if( libSymbol->IsAlias() ) if( libSymbol.IsAlias() )
{ {
LIB_SYMBOL* newCachedSymbol = new LIB_SYMBOL( *libSymbol ); LIB_SYMBOL* newCachedSymbol = new LIB_SYMBOL( libSymbol );
std::shared_ptr<LIB_SYMBOL> bufferedParent = libSymbol->GetParent().lock(); std::shared_ptr<LIB_SYMBOL> bufferedParent = libSymbol.GetParent().lock();
parentSymbol = newCachedSymbol; parentSymbol = newCachedSymbol;
wxCHECK( bufferedParent, false ); wxCHECK( bufferedParent, false );
@ -1080,7 +1072,7 @@ bool LIB_BUFFER::SaveBuffer( std::shared_ptr<SYMBOL_BUFFER> aSymbolBuf,
LIB_SYMBOL& parentRef = *originalParent; LIB_SYMBOL& parentRef = *originalParent;
aSymbolBuf->SetOriginal( std::move( originalParent ) ); aSymbolBuf->SetOriginal( std::move( originalParent ) );
auto newSymbol = std::make_unique<LIB_SYMBOL>( *libSymbol ); auto newSymbol = std::make_unique<LIB_SYMBOL>( libSymbol );
newSymbol->SetParent( &parentRef ); newSymbol->SetParent( &parentRef );
aSymbolBuf->SetOriginal( std::move( newSymbol ) ); aSymbolBuf->SetOriginal( std::move( newSymbol ) );
} }
@ -1102,14 +1094,14 @@ bool LIB_BUFFER::SaveBuffer( std::shared_ptr<SYMBOL_BUFFER> aSymbolBuf,
auto originalBufferedParent = GetBuffer( bufferedParent->GetName() ); auto originalBufferedParent = GetBuffer( bufferedParent->GetName() );
wxCHECK( originalBufferedParent, false ); wxCHECK( originalBufferedParent, false );
auto newSymbol = std::make_unique<LIB_SYMBOL>( *libSymbol ); auto newSymbol = std::make_unique<LIB_SYMBOL>( libSymbol );
newSymbol->SetParent( originalBufferedParent->GetSymbol() ); newSymbol->SetParent( &originalBufferedParent->GetSymbol() );
aSymbolBuf->SetOriginal( std::move( newSymbol ) ); aSymbolBuf->SetOriginal( std::move( newSymbol ) );
} }
} }
else else
{ {
parentSymbol = new LIB_SYMBOL( *libSymbol ); parentSymbol = new LIB_SYMBOL( libSymbol );
try try
{ {
@ -1117,18 +1109,17 @@ bool LIB_BUFFER::SaveBuffer( std::shared_ptr<SYMBOL_BUFFER> aSymbolBuf,
} }
catch( const IO_ERROR& ioe ) catch( const IO_ERROR& ioe )
{ {
wxLogError( errorMsg, UnescapeString( libSymbol->GetName() ), aFileName, wxLogError( errorMsg, UnescapeString( libSymbol.GetName() ), aFileName, ioe.What() );
ioe.What() );
return false; return false;
} }
aSymbolBuf->SetOriginal( std::make_unique<LIB_SYMBOL>( *libSymbol ) ); aSymbolBuf->SetOriginal( std::make_unique<LIB_SYMBOL>( libSymbol ) );
} }
wxArrayString derivedSymbols; wxArrayString derivedSymbols;
// Reparent all symbols derived from the saved symbol. // Reparent all symbols derived from the saved symbol.
if( GetDerivedSymbolNames( libSymbol->GetName(), derivedSymbols ) != 0 ) if( GetDerivedSymbolNames( libSymbol.GetName(), derivedSymbols ) != 0 )
{ {
// Save the derived symbols. // Save the derived symbols.
for( const wxString& entry : derivedSymbols ) for( const wxString& entry : derivedSymbols )
@ -1137,7 +1128,7 @@ bool LIB_BUFFER::SaveBuffer( std::shared_ptr<SYMBOL_BUFFER> aSymbolBuf,
wxCHECK2( symbol, continue ); wxCHECK2( symbol, continue );
LIB_SYMBOL* derivedSymbol = new LIB_SYMBOL( *symbol->GetSymbol() ); LIB_SYMBOL* derivedSymbol = new LIB_SYMBOL( symbol->GetSymbol() );
derivedSymbol->SetParent( parentSymbol ); derivedSymbol->SetParent( parentSymbol );
try try
@ -1163,7 +1154,7 @@ std::shared_ptr<SYMBOL_BUFFER> LIB_BUFFER::GetBuffer( const wxString& aAlias ) c
{ {
for( std::shared_ptr<SYMBOL_BUFFER> entry : m_symbols ) for( std::shared_ptr<SYMBOL_BUFFER> entry : m_symbols )
{ {
if( entry->GetSymbol()->GetName() == aAlias ) if( entry->GetSymbol().GetName() == aAlias )
return entry; return entry;
} }
@ -1175,9 +1166,9 @@ bool LIB_BUFFER::HasDerivedSymbols( const wxString& aParentName ) const
{ {
for( const std::shared_ptr<SYMBOL_BUFFER>& entry : m_symbols ) for( const std::shared_ptr<SYMBOL_BUFFER>& 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. // Check for inherited symbol without a valid parent.
wxCHECK( parent, false ); wxCHECK( parent, false );
@ -1195,11 +1186,13 @@ void LIB_BUFFER::GetSymbolNames( wxArrayString& aSymbolNames, SYMBOL_NAME_FILTER
{ {
for( std::shared_ptr<SYMBOL_BUFFER>& entry : m_symbols ) for( std::shared_ptr<SYMBOL_BUFFER>& entry : m_symbols )
{ {
if( ( entry->GetSymbol()->IsAlias() && ( aFilter == SYMBOL_NAME_FILTER::ROOT_ONLY ) ) const LIB_SYMBOL& symbol = entry->GetSymbol();
|| ( entry->GetSymbol()->IsRoot() && ( aFilter == SYMBOL_NAME_FILTER::DERIVED_ONLY ) ) ) if( ( symbol.IsAlias() && ( aFilter == SYMBOL_NAME_FILTER::ROOT_ONLY ) )
|| ( symbol.IsRoot() && ( aFilter == SYMBOL_NAME_FILTER::DERIVED_ONLY ) ) )
{
continue; 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<SYMBOL_BUFFER>& entry : m_symbols ) for( std::shared_ptr<SYMBOL_BUFFER>& 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. // Check for inherited symbol without a valid parent.
wxCHECK2( parent, continue ); wxCHECK2( parent, continue );
if( parent->GetName() == aSymbolName ) 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; wxArrayString derivedSymbolNames;
std::deque<std::shared_ptr<SYMBOL_BUFFER>>::iterator it; std::deque<std::shared_ptr<SYMBOL_BUFFER>>::iterator it;
if( GetDerivedSymbolNames( aSymbolBuf.GetSymbol()->GetName(), derivedSymbolNames ) ) if( GetDerivedSymbolNames( aSymbolBuf.GetSymbol().GetName(), derivedSymbolNames ) )
{ {
for( const wxString& symbolName : derivedSymbolNames ) for( const wxString& symbolName : derivedSymbolNames )
{ {
it = std::find_if( m_symbols.begin(), m_symbols.end(), it = std::find_if( m_symbols.begin(), m_symbols.end(),
[symbolName]( std::shared_ptr<SYMBOL_BUFFER>& buf ) [symbolName]( std::shared_ptr<SYMBOL_BUFFER>& buf )
{ {
return buf->GetSymbol()->GetName() == symbolName; return buf->GetSymbol().GetName() == symbolName;
} ); } );
wxCHECK2( it != m_symbols.end(), continue ); wxCHECK2( it != m_symbols.end(), continue );

4
eeschema/symbol_library_manager.h
View File

@ -61,10 +61,10 @@ public:
std::unique_ptr<SCH_SCREEN> aScreen = nullptr ); std::unique_ptr<SCH_SCREEN> aScreen = nullptr );
~SYMBOL_BUFFER(); ~SYMBOL_BUFFER();
LIB_SYMBOL* GetSymbol() const { return m_symbol.get(); } LIB_SYMBOL& GetSymbol() const { return *m_symbol; }
void SetSymbol( std::unique_ptr<LIB_SYMBOL> aSymbol ); void SetSymbol( std::unique_ptr<LIB_SYMBOL> aSymbol );
LIB_SYMBOL* GetOriginal() const { return m_original.get(); } LIB_SYMBOL& GetOriginal() const { return *m_original; }
void SetOriginal( std::unique_ptr<LIB_SYMBOL> aSymbol ); void SetOriginal( std::unique_ptr<LIB_SYMBOL> aSymbol );
bool IsModified() const; bool IsModified() const;

10
qa/tests/eeschema/test_symbol_library_manager.cpp
View File

@ -53,8 +53,8 @@ BOOST_AUTO_TEST_CASE( SymbolBuffer )
SYMBOL_BUFFER buffer( std::move( symbol ), std::move( screen ) ); SYMBOL_BUFFER buffer( std::move( symbol ), std::move( screen ) );
BOOST_CHECK( !buffer.IsModified() ); BOOST_CHECK( !buffer.IsModified() );
BOOST_CHECK( buffer.GetSymbol() == &symbolRef ); BOOST_CHECK( &buffer.GetSymbol() == &symbolRef );
BOOST_CHECK( *buffer.GetOriginal() == symbolRef ); BOOST_CHECK( buffer.GetOriginal() == symbolRef );
buffer.GetScreen()->SetContentModified(); buffer.GetScreen()->SetContentModified();
BOOST_CHECK( buffer.IsModified() ); BOOST_CHECK( buffer.IsModified() );
@ -65,9 +65,9 @@ BOOST_AUTO_TEST_CASE( SymbolBuffer )
buffer.SetOriginal( std::move( originalSymbol ) ); buffer.SetOriginal( std::move( originalSymbol ) );
BOOST_CHECK( buffer.GetOriginal() == &originalSymbolRef ); BOOST_CHECK( &buffer.GetOriginal() == &originalSymbolRef );
BOOST_CHECK( *buffer.GetOriginal() == originalSymbolRef ); BOOST_CHECK( buffer.GetOriginal() == originalSymbolRef );
BOOST_CHECK( *buffer.GetSymbol() != *buffer.GetOriginal() ); BOOST_CHECK( &buffer.GetSymbol() != &buffer.GetOriginal() );
} }