KiCad

Bug #1412528
Comment #17

Comment 17 for bug 1412528

Revision history for this message

Alexander Golubev (fat-zer) wrote on 2015-01-28:

#17

Thanks for your review.

2015-01-27 19:17 GMT+03:00 Wayne Stambaugh <email address hidden>:
> Here is my response to your proposed patches:
>
> 0001-EDA_BASE_FRAME-fix-a-Woverloaded-virtual-warning.patch
>
> This is no longer necessary since remove the redundant IsActive()
> definition from EDA_BASE_FRAME.
ok, as you say.

> 0002-LIB_PART-fix-a-Woverloaded-virtual-warning.patch
>
> I don't like adding functions that should never get called (you should
> always specify a aUnit) and possibly confuse developers just to get rid
> of a compiler warning.
>
I'm a bit disagree with that. As I understand zero got its special sense here: it returns the bounding box that fits any variant of the component. And making it the default behavior is straightforward enough and may be useful.
On the other side I suppose it masks a design issue: IMHO each unit should be an individual VIEW_ITEM-derived object. And switch between them should be encapsulated in some other class. I suppose the current implementation is due to historical reasons. But I'm not into kicad enough even to suggest to rework this...

> 0004-A-minor-enhancement-in-EDA_ITEM.patch
>
> This is one of those patches that I really don't see as necessary. Yes,
> technically EDA_ITEM::Matches() only gets call by objects derived from
> EDA_ITEM but doing so buys us nothing in terms of code reliability or
> readability. I may apply this but it would be low on my priority list.
I spent some time wtf'ing, why the heck there are two function and why we pass a string to search inside the object to it. You can believe me that the current state is really confusing. I'm not sure that the proposed patch is the best way to improve the readability but IMO it does so.

> 0005-fix-several-Woverloaded-virtual-warnings-in-common-d.patch
>
> I didn't write this code so I'm not sure that removing these functions
> makes sense. The original author may have intended to use them in the
> future. If the person who wrote it steps up and says that it will never
> be used, I will apply this patch.
>
ok... as Maciej Sumiński sad keep it as is for now...

Thanks for your review.

2015-01-27 19:17 GMT+03:00 Wayne Stambaugh <1412528@bugs.launchpad.net>:
> Here is my response to your proposed patches:
>
> 0001-EDA_BASE_FRAME-fix-a-Woverloaded-virtual-warning.patch
>
> This is no longer necessary since remove the redundant IsActive()
> definition from EDA_BASE_FRAME.
ok, as you say.

> 0004-A-minor-enhancement-in-EDA_ITEM.patch
>
> This is one of those patches that I really don't see as necessary.  Yes,
> technically EDA_ITEM::Matches() only gets call by objects derived from
> EDA_ITEM but doing so buys us nothing in terms of code reliability or
> readability.  I may apply this but it would be low on my priority list.
I spent some time wtf'ing, why the heck there are two function and why we pass a string to search inside the object to it. You can believe me that the current state is really confusing. I'm not sure that the proposed patch is the best way to improve the readability but IMO it does so.

> 0005-fix-several-Woverloaded-virtual-warnings-in-common-d.patch
>
> I didn't write this code so I'm not sure that removing these functions
> makes sense.  The original author may have intended to use them in the
> future.  If the person who wrote it steps up and says that it will never
> be used, I will apply this patch.
>
ok... as Maciej Sumiński sad keep it as is for now...