Project

General

Profile

Actions

Task #2454

closed

code-style: usage of override, final, virtual

Added by Junxiao Shi about 9 years ago. Updated almost 9 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Docs
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
0.50 h

Description

Add a code style rule to: require annotating override or NDN_CXX_DECL_OVERRIDE or DECL_OVERRIDE on a function declaration that overrides a virtual method or destructor, in addition to virtual.

Actions #1

Updated by Junxiao Shi about 9 years ago

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.

Actions #2

Updated by Davide Pesavento about 9 years ago

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.

Actions #3

Updated by Alex Afanasyev about 9 years ago

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.

Actions #4

Updated by Davide Pesavento about 9 years ago

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

Actions #5

Updated by Alex Afanasyev about 9 years ago

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.

Actions #6

Updated by Davide Pesavento about 9 years ago

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

Actions #7

Updated by Davide Pesavento about 9 years ago

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

Actions #8

Updated by Junxiao Shi about 9 years ago

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.
Actions #9

Updated by Davide Pesavento about 9 years ago

ACK

Actions #10

Updated by Junxiao Shi about 9 years ago

  • Status changed from New to In Progress
  • Assignee set to Junxiao Shi
Actions #11

Updated by Junxiao Shi about 9 years ago

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

Actions #12

Updated by Junxiao Shi about 9 years ago

  • Description updated (diff)
  • Status changed from In Progress to Code review
  • Target version changed from v0.3 to v0.4
  • % Done changed from 0 to 100
Actions #13

Updated by Junxiao Shi about 9 years ago

  • Status changed from Code review to Closed
Actions #14

Updated by Junxiao Shi almost 9 years ago

  • Status changed from Closed to Feedback
  • % Done changed from 100 to 80

commit:895395f7f666445712171bb7b7e82387682b3c84 incorrectly declares:

#define NDN_CXX_DECL_FINAL override

This should be final.

Actions #15

Updated by Junxiao Shi almost 9 years ago

  • Status changed from Feedback to Code review
  • % Done changed from 80 to 100
Actions #16

Updated by Junxiao Shi almost 9 years ago

  • Status changed from Code review to Closed
Actions

Also available in: Atom PDF