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@lists.launchpad.net
Unsubscribe : https://launchpad.net/~kicad-developers
More help : https://help.launchpad.net/ListHelp
