p2p: pass spans in CNetAddr by reference to const #22143

pull jonatack wants to merge 1 commits into bitcoin:master from jonatack:netaddress-pass-spans-by-reference-to-const changing 2 files +8 −8

jonatack commented at 3:19 PM on June 3, 2021: contributor

No description provided.
p2p: pass spans in CNetAddr by reference to const c9e73928e6
fanquake added the label P2P on Jun 3, 2021
sipa commented at 3:22 PM on June 3, 2021: member

Hmm, we've been moving in the opposite direction generally in new code, as Spans are sufficiently small (16 bytes on x86_64) that passing them by value is generally preferred.
in src/netaddress.cpp:153 in c9e73928e6
```
 149 | @@ -150,7 +150,7 @@ void CNetAddr::SetIP(const CNetAddr& ipIn)
 150 |      m_addr = ipIn.m_addr;
 151 |  }
 152 |  
 153 | -void CNetAddr::SetLegacyIPv6(Span<const uint8_t> ipv6)
 154 | +void CNetAddr::SetLegacyIPv6(const Span<const uint8_t>& ipv6)
```
laanwj commented at 3:22 PM on June 3, 2021:

I'm not sure this is worth it. span is an extremely light object (a pointer+size), I think it's normally copied by-value because doing an extra dereference on access would be less efficient.

in src/netaddress.cpp:265 in c9e73928e6

 259 | @@ -260,9 +260,9 @@ bool CNetAddr::SetTor(const std::string& addr)
 260 |          m_addr.assign(input.begin(), input.end());
 261 |          return true;
 262 |      case torv3::TOTAL_LEN: {
 263 | -        Span<const uint8_t> input_pubkey{input.data(), ADDR_TORV3_SIZE};
 264 | -        Span<const uint8_t> input_checksum{input.data() + ADDR_TORV3_SIZE, torv3::CHECKSUM_LEN};
 265 | -        Span<const uint8_t> input_version{input.data() + ADDR_TORV3_SIZE + torv3::CHECKSUM_LEN, sizeof(torv3::VERSION)};
 266 | +        const Span<const uint8_t> input_pubkey{input.data(), ADDR_TORV3_SIZE};
 267 | +        const Span<const uint8_t> input_checksum{input.data() + ADDR_TORV3_SIZE, torv3::CHECKSUM_LEN};
 268 | +        const Span<const uint8_t> input_version{input.data() + ADDR_TORV3_SIZE + torv3::CHECKSUM_LEN, sizeof(torv3::VERSION)};

ajtowns commented at 3:51 PM on June 3, 2021:

I don't think a const on a non-reference return value (for a copyable object anyway) does anything useful?

jonatack commented at 4:02 PM on June 3, 2021: contributor

Hmm, we've been moving in the opposite direction generally in new code, as Spans are sufficiently small (16 bytes on x86_64) that passing them by value is generally preferred.

Thanks everyone for the feedback. I was wondering which way we should go with these. Closing.
jonatack closed this on Jun 3, 2021
jonatack deleted the branch on Jun 3, 2021
jonatack cross-referenced this on Jun 6, 2021 from issue p2p, rpc, fuzz: various tiny follow-ups by jonatack
fanquake referenced this in commit 1cc123f405 on Jun 7, 2021
sidhujag referenced this in commit 8739d50db8 on Jun 9, 2021
bitcoin locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#22169 p2p, rpc, fuzz: various tiny follow-ups