Using uint8_t for raw bytes has a style benefit:
Other clean-ups in this pull include:
SpanInit() member in favor of C++11 member initialization
Also, rename CSerializeData to SerializeData
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
There are some more unnecessary MakeUCharSpan that can be removed here, especially related to Base64 handling:
0src/rpc/rawtransaction.cpp: return EncodeBase64(MakeUCharSpan(ssTx));
1src/rpc/rawtransaction.cpp: return EncodeBase64(MakeUCharSpan(ssTx));
2src/rpc/rawtransaction.cpp: return EncodeBase64(MakeUCharSpan(ssTx));
3src/rpc/rawtransaction.cpp: return EncodeBase64(MakeUCharSpan(ssTx));
4src/rpc/rawtransaction.cpp: return EncodeBase64(MakeUCharSpan(ssTx));
(possibly more but I could find these quickly)
265- Init(nTypeIn, nVersionIn);
266 ::SerializeMany(*this, std::forward<Args>(args)...);
267 }
268
269- void Init(int nTypeIn, int nVersionIn)
270+ void Rewind()
bool Rewind(size_type n), overloading it here is confusing. We should think of a new name. I had hoped to think of a way to use the existing method in the only place it is used, but it appears there is no way to access nReadPos (or even derive it) from the outside.
Rewind(size_t) rewinds only how much is passed. Rewind() rewinds all the way. Though, I am happy to rename this to anything else, if there are suggestions.
It’s a pity that there is no defined invalid value for size_t, which could be passed as default value. Would Rewind(std::optional<size_t> n = std::nullopt) be a possibility here to unite the two methods? Like e.g.:
0diff --git a/src/streams.h b/src/streams.h
1index 2a921c2c5..a6e781b95 100644
2--- a/src/streams.h
3+++ b/src/streams.h
4@@ -14,6 +14,7 @@
5 #include <assert.h>
6 #include <ios>
7 #include <limits>
8+#include <optional>
9 #include <stdint.h>
10 #include <stdio.h>
11 #include <string.h>
12@@ -238,11 +239,6 @@ public:
13 ::SerializeMany(*this, std::forward<Args>(args)...);
14 }
15
16- void Rewind()
17- {
18- nReadPos = 0;
19- }
20-
21 std::string str() const
22 {
23 return (std::string(begin(), end()));
24@@ -339,12 +335,17 @@ public:
25 nReadPos = 0;
26 }
27
28- bool Rewind(size_type n)
29+ bool Rewind(std::optional<size_type> n = std::nullopt)
30 {
31+ // Total rewind if no size is passed
32+ if (!n) {
33+ nReadPos = 0;
34+ return true;
35+ }
36 // Rewind by n characters if the buffer hasn't been compacted yet
37- if (n > nReadPos)
38+ if (*n > nReadPos)
39 return false;
40- nReadPos -= n;
41+ nReadPos -= *n;
42 return true;
43 }
Probably overkill for methods that are only ever called in the benchmarks though…
I think the name make sense. Rewind(size_t) rewinds only how much is passed.
Right. My remark was triggered mostly by that the two implementations of rewind() were in different places in the method list and not together, the name makes some sense.
But I like the new combined implementation.