kicad-developers team mailing list archive

[Dick Hollenbeck <dick@xxxxxxxxxxx>]

On 02/21/2012 06:00 PM, Janis Skujenieks wrote:

Offering a critique of this patch is perhaps a waste of time if there has been no
agreement on its need.

But for future reference:

a) accessors are to hide data layouts, and protect against silly changes.  ORing in a an
upper bit into the netcode would be fine, but only if this is hidden behind a pair of
accessors.  It is self defeating to do this outside the accessor.  Likewise, your GetNet()
accessor would have to and off the upper bit before returning the netcode.  Hide the shit
inside the accessors please.  Setting a NetCode now has to OR in all the bits except the
upper one.  (In the end, it is probably easier to simply add another bool data field.)

#define OLD_TRACK_NET       0x40000000      // Flag for old netcodes that must be updated

That means this define above should be a secret, behind the 4 accessors which have to know
about it.  Two fields, two accessors each = 4 accessors.



b) typedef on structs are not needed in C++.

c) use spaces, NEVER tabs, indentation of 4.

+	        curr_track->SetNet( 0 );





> === modified file 'pcbnew/connect.cpp'
> --- pcbnew/connect.cpp	2012-02-19 04:02:19 +0000
> +++ pcbnew/connect.cpp	2012-02-21 23:49:28 +0000
> @@ -39,12 +39,23 @@
>  #include <pcbnew.h>
>  
>  
> +
> +#define OLD_TRACK_NET       0x40000000      // Flag for old netcodes that must be updated
> +
> +/// Structure for holding needed substitution data
> +typedef struct NetSubstitutionStruct
> +{
> +	int OldNetcode;
> +	int NewNetcode;
> +}NET_SUBS;



> +
> +
>  extern void Merge_SubNets_Connected_By_CopperAreas( BOARD* aPcb );
>  extern void Merge_SubNets_Connected_By_CopperAreas( BOARD* aPcb, int aNetcode );
>  
>  // Local functions
>  static void RebuildTrackChain( BOARD* pcb );
> -
> +static bool NetSubsCompare( NET_SUBS A, NET_SUBS B );
>  
>  // A helper class to handle connection points (i.e. candidates) for tracks
>  class CONNECTED_POINT
> @@ -969,6 +980,19 @@
>      return;
>  }
>  
> +/**
> + * Function SubsCompare
> + * compares two parameters of type NET_SUBS.
> + *
> + * @param A is first member for comparision.
> + * @param B is second member for comparision.
> + * @return bool, true if A.OldNetcode < B.OldNetcode.
> + */
> +static bool NetSubsCompare( NET_SUBS A, NET_SUBS B )
> +{
> +	if( A.OldNetcode < B.OldNetcode ) return true;
> +	return false;
> +}
>  
>  /* search connections between tracks and pads and propagate pad net codes to the track
>   * segments.
> @@ -977,6 +1001,12 @@
>  void PCB_BASE_FRAME::RecalculateAllTracksNetcode()
>  {
>      TRACK*              curr_track;
> +    NET_SUBS            subst_pair;
> +
> +    /// Stores substitution set for netcodes
> +    std::set<NET_SUBS, bool (*)(NET_SUBS, NET_SUBS)> substitut_set( NetSubsCompare );
> +    std::set<NET_SUBS>::iterator subs_el;
> +
>  
>      // Build the net info list
>      GetBoard()->BuildListOfNets();
> @@ -991,28 +1021,32 @@
>          curr_track->end = NULL;
>          curr_track->SetState( BUSY | IN_EDIT | BEGIN_ONPAD | END_ONPAD, OFF );
>          curr_track->SetZoneSubNet( 0 );
> -        curr_track->SetNet( 0 );    // net code = 0 means not connected
> +        curr_track->SetNet( curr_track->GetNet() | OLD_TRACK_NET );
>      }
> -
> -    // If no pad, reset pointers and netcode, and do nothing else
> -    if( m_Pcb->GetPadCount() == 0 )
> -        return;
> -
> +    
>      CONNECTIONS connections( m_Pcb );
>      connections.BuildPadsList();
> -    connections.BuildTracksCandidatesList(m_Pcb->m_Track);
> +    connections.BuildTracksCandidatesList( m_Pcb->m_Track );
>  
>      // First pass: build connections between track segments and pads.
>      connections.SearchTracksConnectedToPads();
>  
> -    /* For tracks connected to at least one pad,
> -     * set the track net code to the pad netcode
> +    /* If pads > 0, store netcode substitution pairs for tracks
> +     * connected to pads. So later we can change all tracks with this code to
> +     * netcode of pad.
>       */
> -    curr_track = m_Pcb->m_Track;
> -    for( ; curr_track != NULL; curr_track = curr_track->Next() )
> +    if( m_Pcb->GetPadCount() > 0 )
>      {
> -        if( curr_track->m_PadsConnected.size() )
> -            curr_track->SetNet( curr_track->m_PadsConnected[0]->GetNet() );
> +        curr_track = m_Pcb->m_Track;
> +        for( ; curr_track != NULL; curr_track = curr_track->Next() )
> +        {
> +            if( curr_track->m_PadsConnected.size() )
> +            {
> +                subst_pair.OldNetcode = curr_track->GetNet();
> +                subst_pair.NewNetcode = curr_track->m_PadsConnected[0]->GetNet();			
> +                substitut_set.insert(subst_pair);
> +            }
> +        }
>      }
>  
>      // Pass 2: build connections between track ends
> @@ -1022,46 +1056,22 @@
>          connections.GetConnectedTracks( curr_track );
>      }
>  
> -    // Propagate net codes from a segment to other connected segments
> -    bool new_pass_request = true;   // set to true if a track has its netcode changed from 0
> -                                    // to a known netcode to re-evaluate netcodes
> -                                    // of connected items
> -    while( new_pass_request )
> +    // Go trough tracks and apply substitution list or set to 0 if not found
> +    for( curr_track = m_Pcb->m_Track; curr_track != NULL; curr_track = curr_track->Next() )
>      {
> -        new_pass_request = false;
> -
> -        for( curr_track = m_Pcb->m_Track; curr_track; curr_track = curr_track->Next() )
> -        {
> -            int netcode = curr_track->GetNet();
> -            if( netcode == 0 )
> -            {   // try to find a connected item having a netcode
> -                for( unsigned kk = 0; kk < curr_track->m_TracksConnected.size(); kk++ )
> -                {
> -                    int altnetcode = curr_track->m_TracksConnected[kk]->GetNet();
> -                    if( altnetcode )
> -                    {
> -                        new_pass_request = true;
> -                        netcode = altnetcode;
> -                        curr_track->SetNet(netcode);
> -                        break;
> -                    }
> -                }
> -            }
> -            if( netcode )    // this track has a netcode
> -            {   // propagate this netcode to connected tracks having no netcode
> -                for( unsigned kk = 0; kk < curr_track->m_TracksConnected.size(); kk++ )
> -                {
> -                    int altnetcode = curr_track->m_TracksConnected[kk]->GetNet();
> -                    if( altnetcode == 0 )
> -                    {
> -                        curr_track->m_TracksConnected[kk]->SetNet(netcode);
> -                        new_pass_request = true;
> -                    }
> -                }
> -            }
> +        subst_pair.OldNetcode = curr_track->GetNet();
> +        subs_el = substitut_set.find(subst_pair);
> +        
> +        if( subs_el != substitut_set.end() )
> +        {
> +            curr_track->SetNet( (*subs_el).NewNetcode );
> +        }
> +        else
> +        {
> +	        curr_track->SetNet( 0 );
>          }
>      }
> -
> +    
>      // Sort the track list by net codes:
>      RebuildTrackChain( m_Pcb );
>  }
>