This operator has no documented use case and is also unused outside of test code. The test code and all other (imaginary) code that might use this operator is written more clear and concise by the existing CScript push operators for opcodes and data.
Removing the operator is also going to protect against accidentally reintroducing bugs like this https://github.com/bitcoin/bitcoin/commit/6ff5f718b6a67797b2b3bab8905d607ad216ee21#diff-8458adcedc17d046942185cb709ff5c3L1135 (last time it was used).
472@@ -487,15 +473,6 @@ class CScript : public CScriptBase
473 return *this;
474 }
475
476- CScript& operator<<(const CScript& b)
Deleting this is dangerous, as attempts to call it will still work, except they’ll use the function above instead (CScript::operator<<(const std::vector<unsigned char>&)).
I think it could be made into a static check too:
Better solution:
0CScript& operator<<(const CScript& b) = delete;
They shouldn’t use the above. I tested and this is the error:
0error: invalid operands to binary expression ('CScript' and 'CScript')
1
2./script/script.h:431:14: note: candidate function not viable: no known conversion from 'CScript' to 'int64_t' (aka 'long') for 1st argument
3 CScript& operator<<(int64_t b) { return push_int64(b); }
4 ^
5./script/script.h:433:14: note: candidate function not viable: no known conversion from 'CScript' to 'opcodetype' for 1st argument
6 CScript& operator<<(opcodetype opcode)
7 ^
8./script/script.h:441:14: note: candidate function not viable: no known conversion from 'CScript' to 'const CScriptNum' for 1st argument
9 CScript& operator<<(const CScriptNum& b)
10 ^
11./script/script.h:447:14: note: candidate function not viable: no known conversion from 'CScript' to 'const std::vector<unsigned char>' for 1st argument
12 CScript& operator<<(const std::vector<unsigned char>& b)
13 ^
141 error generated.
I think you wanted to say that the non-existent operator should be explicitly deleted just as it has been done for the constructor in e1a55690e66ca962179bc8170695b92af8a3caa8?
If yes, I have done that. The error is now
0error: overload resolution selected deleted operator '<<'
1
2./script/script.h:430:14: note: candidate function has been explicitly deleted
3 CScript& operator<<(const CScript& b) = delete;
4 ^
5./script/script.h:432:14: note: candidate function not viable: no known conversion from 'CScript' to 'int64_t' (aka 'long') for 1st argument
6 CScript& operator<<(int64_t b) { return push_int64(b); }
7 ^
8./script/script.h:434:14: note: candidate function not viable: no known conversion from 'CScript' to 'opcodetype' for 1st argument
9 CScript& operator<<(opcodetype opcode)
10 ^
11./script/script.h:442:14: note: candidate function not viable: no known conversion from 'CScript' to 'const CScriptNum' for 1st argument
12 CScript& operator<<(const CScriptNum& b)
13 ^
14./script/script.h:448:14: note: candidate function not viable: no known conversion from 'CScript' to 'const std::vector<unsigned char>' for 1st argument
15 CScript& operator<<(const std::vector<unsigned char>& b)
16 ^
Checked that with Span, the output looks like this:
0overload resolution selected deleted operator '<<'
1 script << s;
2 ~~~~~~ ^ ~
3./script/script.h:435:14: note: candidate function has been explicitly deleted
4 CScript& operator<<(const CScript& b) = delete;
5 ^
6./script/script.h:452:14: note: candidate function
7 CScript& operator<<(const Span& b)
8 ^
9./script/script.h:437:14: note: candidate function not viable: no known conversion from 'const CScript' to 'int64_t' (aka 'long') for 1st argument
10 CScript& operator<<(int64_t b) { return push_int64(b); }
11 ^
12./script/script.h:439:14: note: candidate function not viable: no known conversion from 'const CScript' to 'opcodetype' for 1st argument
13 CScript& operator<<(opcodetype opcode)
14 ^
15./script/script.h:447:14: note: candidate function not viable: no known conversion from 'const CScript' to 'const CScriptNum' for 1st argument
16 CScript& operator<<(const CScriptNum& b)
17 ^
18./script/script.h:454:14: note: candidate function not viable: no known conversion from 'const CScript' to 'const std::vector<unsigned char>' for 1st argument
19 CScript& operator<<(const std::vector<unsigned char>& b)
20 ^
17@@ -18,10 +18,10 @@ void test_one_input(const std::vector<uint8_t>& buffer)
18 while (fuzzed_data_provider.remaining_bytes() > 0) {
19 switch (fuzzed_data_provider.ConsumeIntegralInRange(0, 7)) {
20 case 0:
21- script += ConsumeScript(fuzzed_data_provider);
22+ (void)ConsumeScript(fuzzed_data_provider);
23 break;
24 case 1:
25- script = script + ConsumeScript(fuzzed_data_provider);
26+ (void)ConsumeScript(fuzzed_data_provider);
I think if you’d merge these two into one call for both case 0 & case 1, the fuzzer might work slightly more efficiently by not making 0 and 1 take a different code path.
Tend to agree. If not going to collapse now could at least add a comment to avoid confusion here
I mean keep case 0 and case 1, but merge them into a single branch.
0case 0:
1case 1:
2 (void)ConsumeScript(fuzzed_data_provider);
operator=(const& and operator=(&& respectively
operator=(const& and operator=(&& respectively
utACK 4444902f5c4df8784cb2b781d82a55a07336e827
It seems like a no-brainer to remove the +/+= if this is all they’re used for. As far as the operator<< is concerned, I believe that replacing any member function with a deleted version never changes semantics, if the result still compiles.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
Concept ACK
Non-existing is better than unused/undocumented :)
305@@ -306,10 +306,10 @@ class TestBuilder
306 return *this;
307 }
308
309- TestBuilder& Add(const CScript& _script)
310+ TestBuilder& Add(const opcodetype& _op)
Opcode
