New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing redundant virtual from override function declaration #5444

Merged
merged 1 commit into from Nov 21, 2018

Conversation

@Mordax
Contributor

Mordax commented Nov 16, 2018

Part of issue #5168.

Description

Removed redundant virtual declaration from functions that already have override stated. As always, let me know if I missed anything.

Checklist

  • Code compiles correctly
  • All tests are passing
  • Changelog entry (if change is visible to the user)
  • Used meaningful commit messages

@Mordax Mordax force-pushed the Mordax:issue-5168-rmvirtual branch from 5d8fb3a to 94f750e Nov 16, 2018

@codecov

This comment was marked as off-topic.

codecov bot commented Nov 16, 2018

Codecov Report

Merging #5444 into develop will increase coverage by <.01%.
The diff coverage is 70.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           develop   #5444      +/-   ##
==========================================
+ Coverage    88.09%   88.1%   +<.01%     
==========================================
  Files          308     308              
  Lines        31247   31182      -65     
  Branches      3751    3745       -6     
==========================================
- Hits         27528   27473      -55     
+ Misses        2465    2455      -10     
  Partials      1254    1254
Flag Coverage Δ
#all 88.1% <70.5%> (ø) ⬆️
#syntax 29.04% <49.64%> (+0.06%) ⬆️

@axic axic requested review from ekpyron and leonardoalt and removed request for ekpyron Nov 16, 2018

@leonardoalt

Minor comment, LGTM.

Show resolved Hide resolved Changelog.md Outdated

@Mordax Mordax force-pushed the Mordax:issue-5168-rmvirtual branch from 94f750e to e19b508 Nov 16, 2018

@Mordax

This comment has been minimized.

Contributor

Mordax commented Nov 16, 2018

I'm not exactly certain what to do about the codecov failing, if someone wants to fill me in on that.

@axic

This comment has been minimized.

Member

axic commented Nov 16, 2018

I was wondering what happens if a subclass so far used virtual override and now it is changed to override only, how does that affect a subclass of the subclass?

Did a tiny experiment and it seems everything is alright, at least on my computer:

#include <stdio.h>

class X {
public:
  virtual unsigned test() { return 1; }
  unsigned testCall() { return test(); }
};

class Y : public X {
public:
  unsigned test() override { return 2; }
};

class Z : public Y {
public:
  unsigned test() override { return 3; }
};

int main() {
  X x;
  Y y;
  Z z;

  printf("%d %d %d\n", x.testCall(), y.testCall(), z.testCall());
}

@christianparpart @ekpyron can you also double check this PR?

@axic axic requested review from ekpyron and christianparpart Nov 16, 2018

@Mordax

This comment has been minimized.

Contributor

Mordax commented Nov 16, 2018

@axic Yeah, cppreference seems to structure it like this, from what I've researched, it seems like the proper way to format it - virtual in base class, override in derived. Although of course, I'd hate to accidentally add in some strange behaviour. Still not sure what the worsened code coverage is all about.

@leonardoalt

This comment has been minimized.

Member

leonardoalt commented Nov 17, 2018

@Mordax you can just ignore codecov ;)

@ekpyron

Indeed even without repeating virtual in a derived class, the function will still be implicitly virtual and can still be overridden in another class deriving from the derived class, so this is OK.

However, I spotted one virtual that was in fact virtual inheritance and not a redundant virtual in an override, so we have to keep that one. Apart from that this looks fine.

Show resolved Hide resolved libdevcore/Exceptions.h Outdated

@christianparpart christianparpart self-assigned this Nov 19, 2018

Show resolved Hide resolved Changelog.md Outdated
Show resolved Hide resolved libdevcore/Exceptions.h Outdated
Removing redundant virtual from override function declaration
Remove trailing whitespace

Remove changelog change

@axic axic force-pushed the Mordax:issue-5168-rmvirtual branch from e19b508 to ea8b7d8 Nov 21, 2018

@axic axic dismissed stale reviews from ekpyron and christianparpart Nov 21, 2018

Fixed.

@axic

axic approved these changes Nov 21, 2018

@axic axic merged commit c9ee302 into ethereum:develop Nov 21, 2018

14 of 16 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci: build_emscripten Your tests passed on CircleCI!
Details
ci/circleci: build_x86_archlinux Your tests passed on CircleCI!
Details
ci/circleci: build_x86_clang7 Your tests passed on CircleCI!
Details
ci/circleci: build_x86_linux Your tests passed on CircleCI!
Details
ci/circleci: build_x86_mac Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: test_buglist Your tests passed on CircleCI!
Details
ci/circleci: test_check_spelling Your tests passed on CircleCI!
Details
ci/circleci: test_check_style Your tests passed on CircleCI!
Details
ci/circleci: test_emscripten_external Your tests passed on CircleCI!
Details
ci/circleci: test_emscripten_solcjs Your tests passed on CircleCI!
Details
ci/circleci: test_x86_archlinux Your tests passed on CircleCI!
Details
ci/circleci: test_x86_linux Your tests passed on CircleCI!
Details
ci/circleci: test_x86_mac Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment