Skip to content
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

[Trivial] Remove dead code #3753

Closed
wants to merge 9 commits into from

Conversation

thejohnfreeman
Copy link
Collaborator

What it says on the tin. Unused methods, undefined declarations, unused members, etc.

@@ -99,9 +99,7 @@ OverlayImpl::Timer::on_timer(error_code ec)
overlay_.m_peerFinder->once_per_second();
overlay_.sendEndpoints();
overlay_.autoConnect();

if ((overlay_.timer_count_ % Tuning::checkIdlePeers) == 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this deleted? This was added so that we don't check for idle peers too frequently.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

timer_count_ is initialized to 0 and never updated. It is always 0, and this condition is always true.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, right! it's a bug then! it should be incremented.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i see what happened. it used to be this:

if ((++overlay_.timer_count_ % Tuning::checkSeconds) == 0)
    overlay_.check();

if ((overlay_.timer_count_ % Tuning::checkIdlePeers) == 0)
    overlay_.deleteIdlePeers();

but then two top lines were deleted in one of the unrelated commits.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem like the right PR to fix that bug, but I wouldn't remove the code either. Let's just keep the code for now and open an issue so we don't lose track of this. @gregtatcam can you open an issue?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened the issue #3754.

Copy link
Collaborator

@seelabs seelabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good - thanks for the cleanup. Left two minor comments.


// Verify backend integrity
backend_->verify();
*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's check with @miguelportilla before removing this and the verify implementation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am okay removing this commented out code. Although the backend verify function isn't used, it could be useful at some point. For instance, the above situation can be addressed by calling verify on the new deterministic backend. I could also see a server startup switch to have the backend be verified at startup.

@@ -99,9 +99,7 @@ OverlayImpl::Timer::on_timer(error_code ec)
overlay_.m_peerFinder->once_per_second();
overlay_.sendEndpoints();
overlay_.autoConnect();

if ((overlay_.timer_count_ % Tuning::checkIdlePeers) == 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem like the right PR to fix that bug, but I wouldn't remove the code either. Let's just keep the code for now and open an issue so we don't lose track of this. @gregtatcam can you open an issue?

Copy link
Contributor

@a-noni-mousse a-noni-mousse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pwang200
Copy link
Contributor

pwang200 commented Feb 4, 2021

As @cryptobrad said LedgerDeltaAcquire::getCountedObjectName and LedgerReplayTaks::getCountedObjectName should be removed. They are not required by CountedObject any more. SkipListAcquire::getCountedObjectName https://github.com/ripple/rippled/blob/80bd107e570b9112de3b2ef52937960986ad5347/src/ripple/app/ledger/impl/SkipListAcquire.h#L112-L116 should also be removed.

Copy link
Collaborator

@seelabs seelabs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Nice cleanups

@thejohnfreeman
Copy link
Collaborator Author

Thanks for the suggestions! I'll work to include them, and another I identified today, so I'll put the PR in draft mode temporarily.

@thejohnfreeman thejohnfreeman marked this pull request as draft February 4, 2021 20:58
Copy link
Collaborator

@gregtatcam gregtatcam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also unused OverlayImpl::serverHandler() and PeerImp::networkID()

@thejohnfreeman thejohnfreeman marked this pull request as ready for review February 5, 2021 01:18
@thejohnfreeman thejohnfreeman added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Feb 12, 2021
This was referenced Jun 2, 2021
@thejohnfreeman thejohnfreeman deleted the dead-code branch June 8, 2021 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants