Several -Woverloaded-virtual warnings

[Alexander Golubev]

Here is a fix for several subj warnings.

The warnings were noted on the mail list before about a year ago: https://lists.launchpad.net/kicad-developers/msg12412.html

There are still some warnings in the the pcbnew/pcbd2kicadpcb_plugin. I haven't touched it because the module lacks of documentation and I don't want to screw it up.

The patches fix at least one minor bug: Find in eesheme haven't searched through error markers with Find command.

[jean-pierre charras (jp-charras)]

Looks outdated.

[Alexander Golubev (fat-zer)]

> Looks outdated.
Nope, you can build the current kicad source with -Woverloaded-virtual either with gcc or clang and see a bunch of those warnings...

[Nick Østergaard (nickoe)]

Fat-Zer are you sure that all your patches apply cleanly against head now? I think I tried the other day from the tar in your original mail, and it did not look like patch was clean.

[Wayne Stambaugh (stambaughw)]

I was looking at patch
0001-EDA_BASE_FRAME-fix-a-Woverloaded-virtual-warning.patch and was
wondering if anyone knows why we are providing our own
EDA_BASE_FRAME::IsActive() instead of using wxFrame::IsActive()? Is
wxFrame::IsActive() broken in some way that we cannot use it to
determine if the wxFrame is active? It appears to me that
EDA_BASE_FRAME::IsActive() and it's related member variable
m_FrameIsActive are unnecessary since I cannot file them used anywhere
in KiCad other than inside an wxActivateEvent() handler which makes them
redundant. Just use wxActiveateEvent::GetActive() instead of keeping a
copy of the active state laying around. Can anyone shed some light on
why this is written this way? I would rather get rid of the unnecessary
code rather than apply this patch.

On 1/21/2015 1:08 PM, Fat-Zer wrote:
>> Looks outdated.
> Nope, you can build the current kicad source with -Woverloaded-virtual either with gcc or clang and see a bunch of those warnings...
>

[jean-pierre charras (jp-charras)]

Le 21/01/2015 19:56, Wayne Stambaugh a écrit :
> I was looking at patch
> 0001-EDA_BASE_FRAME-fix-a-Woverloaded-virtual-warning.patch and was
> wondering if anyone knows why we are providing our own
> EDA_BASE_FRAME::IsActive() instead of using wxFrame::IsActive()? Is
> wxFrame::IsActive() broken in some way that we cannot use it to
> determine if the wxFrame is active? It appears to me that
> EDA_BASE_FRAME::IsActive() and it's related member variable
> m_FrameIsActive are unnecessary since I cannot file them used anywhere
> in KiCad other than inside an wxActivateEvent() handler which makes them
> redundant. Just use wxActiveateEvent::GetActive() instead of keeping a
> copy of the active state laying around. Can anyone shed some light on
> why this is written this way? I would rather get rid of the unnecessary
> code rather than apply this patch.

I perhaps wrote this code.
I do not remember why.

Perhaps it was useful with very old wxWidgets version (I started with 1.66).
Seems now a duplicate of GetActive().

Please feel free to cleanup the code.

Thanks.

>
> On 1/21/2015 1:08 PM, Fat-Zer wrote:
>>> Looks outdated.
>> Nope, you can build the current kicad source with -Woverloaded-virtual either with gcc or clang and see a bunch of those warnings...
>>
>

--
Jean-Pierre CHARRAS

[Wayne Stambaugh (stambaughw)]

On 1/21/2015 1:08 PM, Fat-Zer wrote:
>> Looks outdated.
> Nope, you can build the current kicad source with -Woverloaded-virtual either with gcc or clang and see a bunch of those warnings...
>
Neither 0002-LIB_PART-fix-a-Woverloaded-virtual-warning.patch or
0003-DIALOG_LIB_EDIT_PIN-fix-yet-onoter-Woverloaded-virtu.patch will
apply cleanly to r5383 of the product branch. I didn't look at the
other ones. You may want to update your working branch to r5383 and
double check you patches.

[Alexander Golubev (fat-zer)]

> Fat-Zer are you sure that all your patches apply cleanly against head now?
yep one patch is already applied, the second is partially applied and a yet another one is lost its value...

Here is a new patchset based on current HEAD.

[Wayne Stambaugh (stambaughw)]

On 1/21/2015 2:31 PM, Fat-Zer wrote:
>> Fat-Zer are you sure that all your patches apply cleanly against head now?
> yep one patch is already applied, the second is partially applied and a yet another one is lost its value...
>
> Here is a new patchset based on current HEAD.
>
>
> ** Attachment added: "kicad-patches.tar.bz2"
> https://bugs.launchpad.net/kicad/+bug/1412528/+attachment/4303307/+files/kicad-patches.tar.bz2
>

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.

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.

0003-DIALOG_LIB_NEW_COMPONENT-fix-yet-anoter-Woverloaded-.patch

I will commit this patch.

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.

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.

[Maciej Suminski (orsonmmz)]

Hi Fat-Zer,

Regarding the last patch (0005) - thank you for noting the warnings, but for the time being I would rather leave the code in the current form. Tom is preparing a huge set of patches, that may make the warnings invalid, and if the patch was applied now - it could cause unnecessary conflicts. I will return to the topic, after he finishes his work.

Regards,
Orson

// tagged as 'cern', so it shows up on my to-do list

[Alexander Golubev (fat-zer)]

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...

[Wayne Stambaugh (stambaughw)]

I committed patches 0003 and 0004 in the product branch r5393. Thank you for your contribution to KiCad.

[Maciej Suminski (orsonmmz)]

Just a small update - I have committed the fifth patch in 5483, thank you Alexander.

Regards,
Orson