kicad-developers team mailing list archive

[Henner Zeller <h.zeller@xxxxxxx>]

Thanks for working on cleaning up the code-base by using more
stringent compiler warnings.

While I can't comment on the original question as I don't know the
code too much (I would add ''const' in the Base class), I suggest
something for a styleguide:

   - Always in derived method, prepend it with 'virtual'. That way it
is easy to see if something is intended to be that way ... and easy to
change the base-methods if someone changes the signature. This doesn't
help yet though with compilers checking your intentions.

   - For the compiler being helpful: In C++ 11, there are now the
keywords 'override' and 'final'. So you can write virtual bool
isActive() override { ... }. This helps to find subtle problems if the
base class is changed. So in the case that someone added 'const' in
the derived method, the compiler would complain that there is no
method in the base-class with a signature bool IsActive() const.
  Since not all compilers can do c++11 yet, I would propose that we
start using it, but #define it to an empty string if the c++ compiler
does not implement c++11.

-h


On 18 February 2014 15:41, Sean Silva <chisophugis@xxxxxxxxx> wrote:
> Hi all, I'm just wanting to make sure that some warnings that I'm seeing are
> not intentional before I submit a patch to fix them.
>
> Clang's -Woverloaded-virtual warning [*] fires many times when building,
> primarily of the form:
>
> In file included from
> /home/sean/pg/others/kicad/kicad/eeschema/lib_polyline.cpp:35:
> /home/sean/pg/others/kicad/kicad/include/wxstruct.h:202:10: warning:
>       'EDA_BASE_FRAME::IsActive' hides overloaded virtual function
>       [-Woverloaded-virtual]
>     bool IsActive() const { return m_FrameIsActive; }
>          ^
> /usr/include/wx-2.8/wx/gtk/toplevel.h:69:18: note: hidden overloaded virtual
> function
>       'wxTopLevelWindowGTK::IsActive' declared here: different qualifiers
>       (none vs const)
>     virtual bool IsActive();
>
> Marking this function `const` has unintended consequences for overload
> resolution, and I think that the result is that this code has a bug.
> Basically, the `const` makes the compiler treat it as though it were a
> separate function for the purposes of overriding, which means that the
> subclass's version won't get called through virtual calls. An example of
> this is the following small program:
>
> #include <iostream>
>
> struct Base {
>   virtual bool IsActive() {
>     std::cout << "Base\n";
>     return true;
>   }
> };
>
> struct Derived : public Base {
>   bool IsActive() const {
>     std::cout << "Derived\n";
>     return true;
>   }
> };
>
> int main() {
>   Base *p = new Derived();
>   p->IsActive();
>   return 0;
> }
>
> You might expect it to print "Derived", but it actually prints "Base". The
> fix is simple: remove the `const`.
>
> There are also some -Woverloaded-virtual warnings that fire on stuff like
> like:
>
> /home/sean/pg/others/kicad/kicad/include/plot_common.h:898:18: warning:
>       'DXF_PLOTTER::Text' hides overloaded virtual function
> [-Woverloaded-virtual]
>     virtual void Text( const wxPoint&              aPos,
>                  ^
> /home/sean/pg/others/kicad/kicad/include/plot_common.h:247:18: note: hidden
>       overloaded virtual function 'PLOTTER::Text' declared here: different
> number of
>       parameters (11 vs 10)
>     virtual void Text( const wxPoint&              aPos,
>
> The same issue arises here: DXF_PLOTTER::Text will not be called through a
> virtual call on a PLOTTER, which I think is unintended.
>
> If these warnings seem like unintentional mistakes, I'll go ahead and send
> in a patch fixing them.
>
> [*] I am an LLVM/Clang developer, so my Clang is bleeding edge; this warning
> may not (yet) be in packaged Clang versions.
>
> -- Sean Silva
>
> _______________________________________________
> 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
>