thelsing / knx

knx stack (TP, IP and RF) for arduino and linux, Can be configured with ETS
GNU General Public License v3.0
276 stars 95 forks source link

Removed redundant declaration #199

Closed croghostrider closed 2 years ago

croghostrider commented 2 years ago

Hi thelsing,

it has no effect on the firmware, the compiled firmware is identical.

mumpf commented 2 years ago

Hi @croghostrider,

why do you think this declaration is redundant? It allows to inherit and override according methods. Why should this be removed if inheritence is intended?

Regards, Waldemar

Ing-Dom commented 2 years ago

I did not check for every member if it is the case, BUT, in general, in the derived class the functions does not have to have the virtual specifier, only the base class must.

But, I see no benefit in removing it, because it does not harm but maybe helps understanding the code.

mumpf commented 2 years ago

I am not an expert in C++, so I am happy to learn something. My current understanding is:

Class A with method "virtual x". Class B:A with method "x override" works fine, this is the comment of @SirSydom Class C:B with method "x override" is not possible, because B::x is not virtual. If B would have a method "virtual x override", C would work. So I am talking about multi level inheritence.

If I am wrong, I am fine with this PR. But if I am right, this PR would remove the ability to derive from already derived classes. Such a thing might be intended, but is - as far as I can see - not the case here.

Regards, Waldemar

nanosonde commented 2 years ago

The keywords „override“ and „final“ implicitly assume a virtual method. So removing it is ok.

From my personal experience it depends on the project‘s coding style guidelines if the redundant „virtual“ has to be used or not.

croghostrider commented 2 years ago

Hi, I have opened this pull request for fixing the codefactor styling issues.

@SirSydom l think for better code understand will be better a comment.

For me, it will be fine to leave the redundant override/virtual annotation, but then it should be deactivated this check on codefactor.

thelsing commented 2 years ago

Learned something new. I didn't know that virtual is already implied by override and final, but it makes sense. Thanks for submitting the PR.