refactor: Replace &foo[0] with foo.data() #21817
Conversation
|
Concept ACK
|
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Fun facts:
So the only commits that changes bitcoind are the Span refactor and the dbwrapper refactor. [1] Edit: For g++ I get a small diff: diff --git a/tmp/old/d_objdump b/tmp/new/d_objdump
index 2b7eabdab2..244eabc8de 100644
--- a/tmp/old/d_objdump
+++ b/tmp/new/d_objdump
@@ -871846,7 +871846,7 @@ Disassembly of section .text:
3f7234: cmp $0x3,%eax
3f7237: ja 3f725a <LegacyScriptPubKeyMan::IsMine(CScript const&) const+0x4a>
3f7239: mov %eax,%eax
- 3f723b: lea 0x28389e(%rip),%rdx # 67aae0 <CSWTCH.5100>
+ 3f723b: lea 0x28389e(%rip),%rdx # 67aae0 <CSWTCH.5101>
3f7242: mov (%rdx,%rax,4),%eax
3f7245: mov 0x8(%rsp),%rdx
3f724a: sub %fs:0x28,%rdx
@@ -881655,7 +881655,7 @@ Disassembly of section .text:
4021e3: cmp $0x3,%eax
4021e6: ja 402374 <LegacyScriptPubKeyMan::ImportScriptPubKeys(std::set<CScript, std::less<CScript>, std::allocator<CScript> > const&, bool, long)+0x264>
4021ec: mov %eax,%eax
- 4021ee: lea 0x2788eb(%rip),%rcx # 67aae0 <CSWTCH.5100>
+ 4021ee: lea 0x2788eb(%rip),%rcx # 67aae0 <CSWTCH.5101>
4021f5: mov (%rcx,%rax,4),%eax
4021f8: test %eax,%eax
4021fa: jne 4022bf <LegacyScriptPubKeyMan::ImportScriptPubKeys(std::set<CScript, std::less<CScript>, std::allocator<CScript> > const&, bool, long)+0x1af>
@@ -924216,7 +924216,7 @@ Disassembly of section .text:
42e216: mov 0x168(%rbp),%r12
42e21d: lea 0x158(%rbp),%rax
42e224: mov %rax,(%rsp)
- 42e228: cmp %r12,%rax
+ 42e228: cmp %rax,%r12
42e22b: je 42e25a <CWallet::EncryptWallet(std::__cxx11::basic_string<char, std::char_traits<char>, secure_allocator<char> > const&)+0x4fa>
42e22d: nopl (%rax)
42e230: mov 0x40(%r12),%rdi |
|
Concept ACK |
|
Concept ACK |
|
How about splitting this up into a few more focused PRs? Test and bench changes are unrelated, no? |
Thanks, done in #21840 |
This is confusing at best when parts of a class use the redundant operators and other parts do not.
promag
left a comment
There was a problem hiding this comment.
Concept ACK. Have you thought about some way to prevent new cases? Maybe a linter to detect for [0] with a list of exceptions, if necessary?
|
A linter could be done in a follow-up (not by me, though) |
If anyone wants to write a linter as follow-up then this might be helpful as a starting point: If the consensus opinion is that a linter is needed then I suggest writing it in Python and shelling out to |
|
Code review ACK fac30ee |
I'd be very happy to work on this if there is agreement there should be a lint test for it. If we want to create a new python based linter for it, it could also make sense to consolidate some of the existing cpp lint tests into it such as: |
|
cr ACK fac30ee: patch looks correct |
Yes I like this line of thinking. |
src/script/interpreter.cpp
Outdated
There was a problem hiding this comment.
Fun fact: If CScript was a std::vector, this would be UB:
$ ./src/bitcoin-cli sendrawtransaction 020000000001015f784e93f6b843bd9ad1b34f86f0a7b034053ac5f18da5c8547aa6041ba24d3d0000000000ffffffff013905000000000000220020e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855010000000000
/usr/include/c++/11/debug/vector:438:
In function:
std::__debug::vector<_Tp, _Allocator>::reference
std::__debug::vector<_Tp,
_Allocator>::operator[](std::__debug::vector<_Tp,
_Allocator>::size_type) [with _Tp = unsigned char; _Allocator =
std::allocator<unsigned char>; std::__debug::vector<_Tp,
_Allocator>::reference = unsigned char&; std::__debug::vector<_Tp,
_Allocator>::size_type = long unsigned int]
Error: attempt to subscript container with out-of-bounds index 0, but
container only holds 0 elements.
Objects involved in the operation:
sequence "this" @ 0x0x7fb68e7f8670 {
type = std::__debug::vector<unsigned char, std::allocator<unsigned char> >;
}
error: Could not connect to the server 127.0.0.1:18443 (error code 1 - "EOF reached")
Make sure the bitcoind server is running and that you are connecting to the correct RPC port.
[1]+ Aborted (core dumped) ./src/bitcoind -noprinttoconsole
The main theme of this refactor is to replace
&foo[0]withfoo.data().The first commit is taken from #21781 with the rationale:
The other commits aim to remove all
&foo[0], wherefoois any kind of byte representation. The rationale:foo.data()should be preferred, as pointed out in the developer notes. This addresses the instances that have been missed in commit 592404f, and Fixes subscript 0 (&var[0]) where should use (var.data()) instead. #9804