kicad-developers team mailing list archive

[Tomasz Wlostowski <tomasz.wlostowski@xxxxxxx>]

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