Skip to content

Conversation

@vasild
Copy link
Contributor

@vasild vasild commented Jan 13, 2023

At the time when

pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg, pnode->vRecvMsg.begin(), it);

is called, it is certainly pnode->vRecvMsg.end() which makes the call equivalent to:

pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg, pnode->vRecvMsg.begin(), pnode->vRecvMsg.end());

which is equivalent to:

pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg);

Thus, use the latter. Further, maybe irrelevant, but the latter has constant complexity while the original code is O(length of vRecvMsg).

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 13, 2023

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK theStack, MarcoFalke, jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the P2P label Jan 13, 2023
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

Concept ACK

Could apply the same simplifcation to ConnmanTestMsg::NodeReceiveMsgBytes (in ./src/test/util/net.cpp)?

At the time when

```cpp
pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg, pnode->vRecvMsg.begin(), it);
```

is called, `it` is certainly `pnode->vRecvMsg.end()` which makes the
call equivalent to:

```cpp
pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg, pnode->vRecvMsg.begin(), pnode->vRecvMsg.end());
```

which is equivalent to:

```cpp
pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg);
```

Thus, use the latter. Further, maybe irrelevant, but the latter has
constant complexity while the original code is `O(length of vRecvMsg)`.
@vasild vasild force-pushed the simplify_vProcessMsg_splice branch from f58fe9c to dfc01cc Compare January 30, 2023 10:21
@vasild
Copy link
Contributor Author

vasild commented Jan 30, 2023

f58fe9cf9c...dfc01ccd73:

Could apply the same simplifcation to ConnmanTestMsg::NodeReceiveMsgBytes

Good catch, done!

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

Code-review ACK dfc01cc

@maflcko
Copy link
Member

maflcko commented Jan 30, 2023

Looks like this was made superfluous in 6294ecd

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

review ACK dfc01cc 🐑

Show signature

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK dfc01ccd73e1f12698278d467c241f398da9fc7d   🐑
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjVGgv/Qo2DWDrtuhClKV9XUnELE8lGhcBjyxWGWlgWnW8XgD3q+GwQy0/lM0N8
DauZKO2j+4EzqBu/zjZdGtKOjTHs5dfErbESwI2JyUIDRwDJ6R+SLOzcnqeIxUTl
NSvJzvAPDfwXawgG+8xYGhlGG1JuZkb6qqeXXT3sYz71FoodKY32F3dwknFRgUHa
NlqlJcFkf8MyQN0pAknGhbf1EU1QnQ1ThMobdiSpXn5GofFaxJ/HXBOSitdhbpIF
A3urTWbWkIaDghLcl1a7e7nC5yw/aOuLx5KjMz7AE7C3tu1zA3viAbokhdlKAFhC
3Eh6RUULHv7AYi/kdCJWs26TI609eMEYCOFrbiFGZCftAR/DFoxQRzBS1sEVp0ps
eSZC7LRck0p8K/HPdZd1/+c58IfCgG8SfyjQpaywXXSExak/9AzHilGXiSmMGIGI
Zk3W5PZd0U1Aru3xUdbsbHQdgZdPGfEHV4lWXRhz7u3fzd6AQuBnieNcnEz/CWhZ
L18MDwXc
=W9U9
-----END PGP SIGNATURE-----

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Light review ACK dfc01cc

@maflcko maflcko merged commit ba39ffe into bitcoin:master Feb 1, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 1, 2023
dfc01cc net: simplify the call to vProcessMsg.splice() (Vasil Dimov)

Pull request description:

  At the time when

  ```cpp
  pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg, pnode->vRecvMsg.begin(), it);
  ```

  is called, `it` is certainly `pnode->vRecvMsg.end()` which makes the call equivalent to:

  ```cpp
  pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg, pnode->vRecvMsg.begin(), pnode->vRecvMsg.end());
  ```

  which is equivalent to:

  ```cpp
  pnode->vProcessMsg.splice(pnode->vProcessMsg.end(), pnode->vRecvMsg);
  ```

  Thus, use the latter. Further, maybe irrelevant, but the latter has constant complexity while the original code is `O(length of vRecvMsg)`.

ACKs for top commit:
  theStack:
    Code-review ACK dfc01cc
  MarcoFalke:
    review ACK dfc01cc   🐑
  jonatack:
    Light review ACK dfc01cc

Tree-SHA512: 9f4eb61d1caf4af9a61ba2f54b915fcfe406db62c58ab1ec42f736505b6792e9379a83d0458d6cc04f289edcec070b7c962f94a920ab51701c3cab103152866f
@bitcoin bitcoin locked and limited conversation to collaborators Feb 1, 2024
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.

5 participants