This was done in the context of #25284 , but I think it also makes sense standalone.
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for AutoFile and HashVerifier. CAutoFile and CHashVerifier remain in places where it is not yet possible.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | stickies-v |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
It is similar to CHashVerifier, but HashVerifier does not need a
serialize type and version
165 | @@ -165,6 +166,39 @@ class CHashWriter : public HashWriter 166 | }; 167 | 168 | /** Reads data from an underlying stream, while hashing the read data. */ 169 | +template <typename Source> 170 | +class HashVerifier : public HashWriter
I'm not sure it's desirable, I know multiple inheritance can be a bit of a mess, and frankly I'm too unfamiliar with it to assess whether or not this is a better approach. Just thought I'd share since it does lead to a fair amount of code deduplication. Since it's a transitory phase, I think it might make sense. Compiles and unit tests are fine:
<details> <summary>git diff</summary>
diff --git a/src/hash.h b/src/hash.h
index b2ef29fcd..05d72dbab 100644
--- a/src/hash.h
+++ b/src/hash.h
@@ -145,7 +145,7 @@ public:
}
};
-class CHashWriter : public HashWriter
+class CHashWriter : virtual public HashWriter
{
private:
const int nType;
@@ -167,7 +167,7 @@ public:
/** Reads data from an underlying stream, while hashing the read data. */
template <typename Source>
-class HashVerifier : public HashWriter
+class HashVerifier : virtual public HashWriter
{
private:
Source& m_source;
@@ -200,29 +200,10 @@ public:
};
template<typename Source>
-class CHashVerifier : public CHashWriter
+class CHashVerifier : public CHashWriter, public HashVerifier<Source>
{
-private:
- Source* source;
-
public:
- explicit CHashVerifier(Source* source_) : CHashWriter(source_->GetType(), source_->GetVersion()), source(source_) {}
-
- void read(Span<std::byte> dst)
- {
- source->read(dst);
- this->write(dst);
- }
-
- void ignore(size_t nSize)
- {
- std::byte data[1024];
- while (nSize > 0) {
- size_t now = std::min<size_t>(nSize, 1024);
- read({data, now});
- nSize -= now;
- }
- }
+ explicit CHashVerifier(Source* source) : CHashWriter(source->GetType(), source->GetVersion()), HashVerifier<Source>(*source) {}
template<typename T>
CHashVerifier<Source>& operator>>(T&& obj)
</details>
CHashVerifier will (might) be removed anyway, so I think temporarily having 10 lines of additional code should be fine for now
Fair point, can be marked resolved.
460 | return error("%s: OpenUndoFile failed", __func__);
461 | }
462 |
463 | // Write index header
464 | - unsigned int nSize = GetSerializeSize(blockundo, fileout.GetVersion());
465 | + unsigned int nSize = GetSerializeSize(blockundo, CLIENT_VERSION);
nit
unsigned int nSize{GetSerializeSize(blockundo, CLIENT_VERSION)};
Thanks, will do on the next push, if there is one.
Concept ACK
165 | @@ -165,6 +166,39 @@ class CHashWriter : public HashWriter 166 | }; 167 | 168 | /** Reads data from an underlying stream, while hashing the read data. */ 169 | +template <typename Source> 170 | +class HashVerifier : public HashWriter 171 | +{ 172 | +private: 173 | + Source& m_source;
Shouldn't this ideally be const? Can be done when making AutoFile::read() const.
<details> <summary>git diff</summary>
diff --git a/src/hash.h b/src/hash.h
index b2ef29fcd..ef5f03578 100644
--- a/src/hash.h
+++ b/src/hash.h
@@ -170,10 +170,10 @@ template <typename Source>
class HashVerifier : public HashWriter
{
private:
- Source& m_source;
+ const Source& m_source;
public:
- explicit HashVerifier(Source& source LIFETIMEBOUND) : m_source{source} {}
+ explicit HashVerifier(const Source& source LIFETIMEBOUND) : m_source{source} {}
void read(Span<std::byte> dst)
{
diff --git a/src/streams.h b/src/streams.h
index 4f2c3ffe7..ff812280f 100644
--- a/src/streams.h
+++ b/src/streams.h
@@ -516,7 +516,7 @@ public:
//
// Stream subset
//
- void read(Span<std::byte> dst)
+ void read(Span<std::byte> dst) const
{
if (!file) throw std::ios_base::failure("AutoFile::read: file handle is nullptr");
if (fread(dst.data(), 1, dst.size(), file) != dst.size()) {
</details>
My preference would be to do it for all functions in the same commit: git grep 'void read(Span<'
ACK eeee61065fe165dcce9625f7cc4cfce9e432aafa
Obviously not against getting this merged, but I think I'd have been more comfortable with another ACK? Perhaps someone can have a look here post-merge? Not a huge overhaul, but also not as trivial as a docs change.
utACK eeee61065fe165dcce9625f7cc4cfce9e432aafa
