kicad-developers team mailing list archive

[Wayne Stambaugh <stambaughw@xxxxxxxxx>]

It looks like the PCB_COLLECTOR objects are not swigged yet but that
would be the way to do it.  Properly swigged, there would be nothing
preventing you from collecting all objects connected to an net or any
other collection criteria for that matter.  That's what the collector
object is for.  Let's not reinvent the wheel.

On 11/15/2017 10:59 AM, Tomasz Wlostowski wrote:
> On 15/11/17 15:37, miles mccoo wrote:
>> Hello,
>>
> Hello Miles,
> 
> Thanks for the patch. My comments below:
> 
>>
>> A couple code review type observations on the existing code.
>>
>>  1. The new connectivity stuff is in connectivity.h. Perhaps it should
>>     be called class_connectivity.h to match the naming of other files?
>>     better yet class_connectivity_data.h
> Nope. Ideally, we should have one class-one file mapping, we're not
> there yet. Even the files that are prefixed with class_* don't follow
> the rule strictly (e.g. CLASS_BOARD). I'd rather rename it to
> connectivity_data.h to match the inner class name.
> 
> 
>>  2. The initial commit comment of connectivity.h should says this:
>>     "New connectivity algorithm."
>>     It would have been helpful to have more. Presumably, there's a
>>     design document somewhere describing the change. Or perhaps and
>>     email thread? Could have saved some hunting.
> 
>>  3. The API that I'm exposing in python is this one:
>>     const std::list<BOARD_CONNECTED_ITEM*> GetNetItems( int aNetCode,
>>                                            const KICAD_T aTypes[] ) const;
>>
>>     And is used like this:
>>     const KICAD_T types[] = { PCB_PAD_T, PCB_ZONE_AREA_T, EOT };
>>     auto netItems = m_connectivity->GetNetItems( i, types );
>>
>>     why not just use a std::vector? Adding the EOT is another thing
>>     folks have to remember to include.
> I'm OK with that. Feel free to add convenience wrappers for Python (e.g.
> BOARD::GetNetItems, etc.) in the swig interface files.
> 
> Concerning the use of std::list - it's there probably because the
> previous ratsnest code (RN_DATA) returned a std::list. I don't mind
> changing the function to return std::vector. Could you update your patch
> when it's done?
> 
>>  4. m_connectivity is a public member of connectivity_data and used all
>>     over the place. My head is ringing with the voices of my college TAs
>>     about access methods. 
> Nope, it's not. I don't know what version of Kicad code you looked at,
> but certainly in the current master m_connectivity is private.
> 
> Tom
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
>