kicad-developers team mailing list archive

[Dick Hollenbeck <dick@xxxxxxxxxxx>]

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.

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.

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.


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


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.  I remain
opposed to including the virtual in the derived classes also however.


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.