kicad-developers team mailing list archive

[Sean Silva <chisophugis@xxxxxxxxx>]

On Tue, Feb 18, 2014 at 7:07 PM, Henner Zeller <h.zeller@xxxxxxx> wrote:

> 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),


The base class is in a system header so that isn't possible in this case.


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

Yeah, that is what we do in LLVM/Clang (the codebase that I work on
primarily). If we have to make a de-novo decision for the KiCad project, I
would oppose using `virtual` since as you mention it doesn't actually do
anything.

-- Sean Silva


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