NFD » ndn-cxx

This was suggested by @Davide in Change I5539565d2dd238d1421f5bd828fde4ecfba0a820 patchset 1 daemon/fw/retx-suppression-fixed.hpp line 46.

I agree with requiring DECL_OVERRIDE.

I'm indifferent to whether virtual should be used when a method already has DECL_OVERRIDE.

Yeah, my opinion is that virtual is completely unnecessary, even for a human, and creates visual clutter when the method/destructor is marked DECL_OVERRIDE. And DECL_OVERRIDE should be required on overrides because it enables stricter compile-time checks. Therefore virtual should be forbidden for overrides.

I would defer the discussion on final until we actually need it, but rules similar to override/DECL_OVERRIDE probably make sense.

I would not remove usage of virtual. Even though it is duplicate, it is more standard way of doing things and is more visible (is is upfront to the declaration).

DECL_OVERRIDE is our own macro which may or may not expand to c++11 override, which may or may not be familiar to people reading the code and trying to understand it.

In short, I'm in opinion of using both.

Alex Afanasyev wrote:

I would not remove usage of virtual. Even though it is duplicate, it is more standard way of doing things

There is no "more" or "less" standard. Something is either standard or it's not. override is in the ISO C++ standard, so it's definitely standard. DECL_OVERRIDE is not standard, but we use plenty of non-standard macros throughout the codebase, so I'd say this is not a valid argument.

and is more visible (is is upfront to the declaration).

This is very personal I guess, but I believe the virtual at the beginning is in fact distracting and creates visual clutter. Also, some editors/IDEs are able to highlight virtual methods in derived classes without explicit marking (mine uses an italic font).

DECL_OVERRIDE is our own macro which may or may not expand to c++11 override,

This is irrelevant to both compilers and human readers.

which may or may not be familiar to people reading the code and trying to understand it.

True, but it applies to tons of other things in the code...

I would not remove usage of virtual. Even though it is duplicate, it is more standard way of doing things

There is no "more" or "less" standard. Something is either standard or it's not. override is in the ISO C++ standard, so it's definitely standard. DECL_OVERRIDE is not standard, but we use plenty of non-standard macros throughout the codebase, so I'd say this is not a valid argument.

By "more" standard I meant that for a pre C++11 person, virtual is easily recognizable thing, while "override" will require at least googling. And given it is not just "override", but DECL_OVERRIDE, he/she will not be able to easily find this out.

and is more visible (is is upfront to the declaration).

This is very personal I guess, but I believe the virtual at the beginning is in fact distracting and creates visual clutter. Also, some editors/IDEs are able to highlight virtual methods in derived classes without explicit marking (mine uses an italic font).

This is personal preference. I wouldn't rely on editors be very smart, given in many cases code is looked up online (gerrit/github).

DECL_OVERRIDE is our own macro which may or may not expand to c++11 override,

This is irrelevant to both compilers and human readers.

It is very much relevant for human readers and compilers. gcc46 not seeing virtual may not produce warnings that could have solved the problem. Human eyes not trained to understand obscure DECL_OVERRIDE (and editors must be really smart to do macro expansion for syntax highlighting) may miss some obvious problems.

which may or may not be familiar to people reading the code and trying to understand it.

True, but it applies to tons of other things in the code...

Yes and if it was possible (unfortunately it is not), I would like to minimize those at least inside interface part of the code.

Alex Afanasyev wrote:

DECL_OVERRIDE is our own macro which may or may not expand to c++11 override,

This is irrelevant to both compilers and human readers.

It is very much relevant for human readers and compilers. gcc46 not seeing virtual may not produce warnings that could have solved the problem.

Nope, this is not the case. (the whole reason why override was invented and added to C++11 is that this kind of problems used to be unfeasible to diagnose)

Human eyes not trained to understand obscure DECL_OVERRIDE (and editors must be really smart to do macro expansion for syntax highlighting) may miss some obvious problems.

This is why we have CI :)

Anyway, I won't argue further about the redundancy of virtual, it's a minor thing.

So, the decision is:

• virtual only indicates a method can be overridden in subclass, and the same method doesn't exist in superclass.
• virtual and DECL_OVERRIDE indicate a method is overriding the same method in superclass, and it can be overridden again in subclass.
• virtual and DECL_FINAL indicate a method is overriding the same method in superclass, and it cannot be overridden again in subclass.

ACK

Before introducing the rule, the build script needs to be prepared with the declaration that expands to 'override' and 'final'.

http://gerrit.named-data.net/1837