Skip to content

Conversation

@mmachicao
Copy link
Contributor

IsReachable is the inverse of IsLimited, but the implementation is duplicated (DRY)

  • Changed the implementation accordingly.
  • Added unittests to document behavior and relationship
  • Unittests have to reset values because of internal statefull variables.
  • My modification in net.cpp applies only to IsReachable.
  • Applied clang -> (unfortunately) lots of changes on formatting

{
if (fListen && pnode->fSuccessfullyConnected)
{
if (fListen && pnode->fSuccessfullyConnected) {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi MarkoFalke. I am afraid, appveyor will reject a file that is not fully formatted according to clang rules. Better bite the bullet now?

Copy link
Member

Choose a reason for hiding this comment

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

We don't check formatting in appveyor. Do you have a failing build to link to?

@fanquake fanquake added the Tests label Dec 27, 2018
@DrahtBot
Copy link
Contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #14929 (net: Allow connections from misbehavior banned peers by gmaxwell)
  • #14897 (randomize GETDATA(tx) request order and introduce bias toward outbound by naumenkogs)
  • #14358 ([logs] Fix a few log messages related to duration measurement by romanz)
  • #14336 (net: implement poll by pstratem)
  • #14033 (p2p: Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater by Empact)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@mmachicao
Copy link
Contributor Author

Closing this one. Am providing another pull request with the minimal change set.

Note: Jsut reviewed. The conflicts with other pull requests are related exclusively to the application of clang format. Will create a pull request with clang format exclusively for my changed lines.

@mmachicao mmachicao closed this Dec 28, 2018
laanwj added a commit that referenced this pull request Jan 9, 2019
…ncludes unit tests

6dc4593 IsReachable is the inverse of IsLimited (DRY). Includes unit tests (marcaiaf)

Pull request description:

  IsReachable is the inverse of IsLimited, but the implementation is duplicated (DRY)

  - Changed the implementation accordingly.
  - Added unit tests to document behavior and relationship
  - My modification in net.cpp  applies only to IsReachable.
  - Applied clang-format-diffpy

  Created new pull request to avoid the mess with:
  #15044

  Checked with supposedly conflicting PRs mentioned in the old PR. No conflicts with the specific changes in this PR.

Tree-SHA512: b132dec6cc2c788ebe4f63f228d78f441614e156743b17adebc990de0180a5872874d2724c86eeaa470b4521918bd137b0e33ebcaae77c5efc1f0d56104f6c87
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 28, 2021
…DRY). Includes unit tests

6dc4593 IsReachable is the inverse of IsLimited (DRY). Includes unit tests (marcaiaf)

Pull request description:

  IsReachable is the inverse of IsLimited, but the implementation is duplicated (DRY)

  - Changed the implementation accordingly.
  - Added unit tests to document behavior and relationship
  - My modification in net.cpp  applies only to IsReachable.
  - Applied clang-format-diffpy

  Created new pull request to avoid the mess with:
  bitcoin#15044

  Checked with supposedly conflicting PRs mentioned in the old PR. No conflicts with the specific changes in this PR.

Tree-SHA512: b132dec6cc2c788ebe4f63f228d78f441614e156743b17adebc990de0180a5872874d2724c86eeaa470b4521918bd137b0e33ebcaae77c5efc1f0d56104f6c87
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants