kicad-developers team mailing list archive

[Henner Zeller <h.zeller@xxxxxxx>]

On 19 February 2014 04:17, Dick Hollenbeck <dick@xxxxxxxxxxx> wrote:
>
>
> HZ> Thanks for working on cleaning up the code-base by using more
> stringent compiler warnings.
>
>
> DH> NO.  The best thing that could happen to clean up the code base would be for
> Jean-Pierre to read and follow the coding standards.

:) Sure, but if we can make sure that some common mistakes are not
happening due to warnings, that would be helpful as well, no ?

>
> a) Automatics starting with capital letters, countless
>
>
> b)
> {
> public:
>
> public:
>
> }
>
>
> doubly public declaraions within classes......    Wayne and I have spent years and a
> fortune cleaning this up, and it still comes in at a pace that I can no longer stomach.
> THAT is the single biggest way to clean up the code base.

Agree on this one, some classes could use some .. cleanup work :)

>
> Some compiler warnings are like your little brother running down the hallway with a tuning
> fork yelling "out of tune" while you are serenading your girlfriend with a guitar in the
> bedroom.

Yes, being reminded after a long time of neglect of tune is of course annoying.
But - consider how much better the serenade would be if your brother
would have told you while preparing for this moment :)

This is what the compiler warnings want to help tell you so that the
code-base is less likely getting out of shape.

>
>
> HZ> 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.
>
>
> DH> NO.  The virtual-ness is inherited.  That is sufficient.  The OP seems to not
> understand, like you do, that the virtual ness is inherited, and affects all calls to the
> similarly named functions in the derived classes.  (His concern about const-ness coming
> and going is valid however, and is the essence of the risk we are trying to avoid.)

Yes, the virtualness is inherited, we all know that. Including the OP
who actually works on a C++ compiler :)

Writing 'virtual' in front of a derived method is just a courtesy to
the reader of the code of the derived class. That way, if someone
attempts to change a method, they are alarmed that they might want to
check in the base-class if it is coming from there. It is generally
good coding style to write the 'virtual' in front of derived classes
to inform programmers (that, unlike the compiler, doesn't necessarily
see the level up).

I agree with you that we might not need that when we use 'override' or
'final', though still it is good for readability.

You haven't explained yet why you are opposed to document the
virtualness by adding it to the method ? You say you dont 'like' it,
but why ?

(Note, that all that has NOTHING to do with the -Woverloaded-virtual
this thread is about: they most likely indicate ACTUAL problems)

>
>
> HZ>   - 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.
>
>
> DH> ALMOST.  This is an interesting idea, but I still don't like the virtual in the
> derived class.  The override keyword however is sufficient:
>
>
>   http://en.cppreference.com/w/cpp/language/override
>
>
> In the example here, it does precisely what it does without virtual again.  And you can
> find that in the code now by grepping for comments including "overload", which is what I
> put in place as I began to remove the virtuals from derived classes.
>
> So Henner, I like your last suggestion about using the
>
>   #define override      // nothing, compiler is not C++11
>
> for old school compilers.  That would allow us to start using the new keyword.

Ok, I'll start looking for step-by-step patch opportunities here and
send them to the list.

>  I remain
> opposed to including the virtual in the derived classes also however.

Ok, got that. A little explanation would be good though (I explained
that I think it is good, because it is serving as a documentation, but
I can't really come up with an example how the opposite can help the
reader (and I am not talking about the compiler, it doesn't care)).


>
> A refinement on the idea would be to use a
>
>   #define OVERLOAD  override
> #else
>   #define OVERLOAD  // nothing
>
>
> so you can turn it on and off from one site, or in one class.
>
>
> Dick
>
>
> P.S.
>
> I am not really on this mailing list due to a bug at launchpad.  I can send email to it,
> but not receive email.  That makes it hard to participate in ANY threads.  Seems like a
> launchpad bug to me.
>
>
>
>
>
> _______________________________________________
> 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