refactor: Use AutoFile and HashVerifier (without ser-type and ser-version) where possible #26649
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
added 2 commits
January 3, 2023 12:54
It is similar to CHashVerifier, but HashVerifier does not need a serialize type and version
fa55c7e to
eeee610
Compare
stickies-v
reviewed
Jan 26, 2023
| if (fileout.IsNull()) { | ||
| return error("%s: OpenUndoFile failed", __func__); | ||
| } | ||
|
|
||
| // Write index header | ||
| unsigned int nSize = GetSerializeSize(blockundo, fileout.GetVersion()); | ||
| unsigned int nSize = GetSerializeSize(blockundo, CLIENT_VERSION); |
There was a problem hiding this comment.
nit
Suggested change
| unsigned int nSize = GetSerializeSize(blockundo, CLIENT_VERSION); | |
| unsigned int nSize{GetSerializeSize(blockundo, CLIENT_VERSION)}; |
There was a problem hiding this comment.
Thanks, will do on the next push, if there is one.
stickies-v
approved these changes
Jan 30, 2023
| class HashVerifier : public HashWriter | ||
| { | ||
| private: | ||
| Source& m_source; |
There was a problem hiding this comment.
Shouldn't this ideally be const? Can be done when making AutoFile::read() const.
git diff
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()) {There was a problem hiding this comment.
My preference would be to do it for all functions in the same commit: git grep 'void read(Span<'
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jan 30, 2023
… ser-type and ser-version) where possible eeee610 Use AutoFile and HashVerifier where possible (MarcoFalke) fa96114 Add HashVerifier (MarcoFalke) Pull request description: This was done in the context of bitcoin#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. ACKs for top commit: stickies-v: ACK eeee610 Tree-SHA512: 93786778c309ecfdc1ed43552d24ff9d966954d69a47f66faaa6de24daacd25c651f3f62bde5abbb362700298fb3c04ffbd3207a0dd13d0bd8bff7fd6d07dcf8
|
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 eeee610 |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
AutoFileandHashVerifier.CAutoFileandCHashVerifierremain in places where it is not yet possible.