kicad-developers team mailing list archive

[Sean Silva <chisophugis@xxxxxxxxx>]

On Wed, Feb 19, 2014 at 7:17 AM, 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.
>
> 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,


There must have been some misunderstanding if it seemed that I didn't know
that virtual-ness is inherited (I'm actually a C++ compiler developer).
>From your earlier email it seems like you think that this warning is trying
to tell you that you need to put `virtual` before overriden functions in
subclasses; this is not the case (see below for a better explanation).

It may be that the terms overload vs. override have different
meanings; EDA_BASE_FRAME::IsActive
overloads but does not override wxTopLevelWindowGTK::IsActive, due to the
additional `const`.


> and affects all calls to the
> similarly named functions in the derived classes.


I think you misunderstood my explanation of what the warning is trying to
protect against. This warning exists precisely because it is not in fact
true that all "similarly named" functions in derived classes are affected
in this way: there are a couple subtle things which can cause it to not be
true. In this case, the differing `const` makes the compiler treat it as
though it were a function with a completely different name, which is the
essence of the confusion that this warning is trying to prevent.

Said another way, would expect the code still work as intended if:
- You renamed EDA_BASE_FRAME::IsActive to EDA_BASE_FRAME::AnotherName
- Make a similar rename in all the sub classes of EDA_BASE_FRAME
- But *not* rename wxTopLevelWindowGTK::IsActive
?

If yes, then that's fine, although the choice of naming is somewhat
confusing. I'll go ahead and just disable the warning then.
If no, then the code as currently written is not working as intended,
because this transformation would in fact not change the behavior of the
code.

Hopefully this clears up any misunderstanding.

-- Sean Silva


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