Remove unused float serialization #21981
Conversation
|
Good catch! Code review ACK fa57a8f |
|
Nice! cr ACK fa57a8f |
|
Concept ACK, certainly no need to keep this longer if it's unused. |
|
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. |
| @@ -252,7 +239,7 @@ template<typename Stream> inline void Unserialize(Stream& s, int32_t& a ) { a = | |||
| template<typename Stream> inline void Unserialize(Stream& s, uint32_t& a) { a = ser_readdata32(s); } | |||
| template<typename Stream> inline void Unserialize(Stream& s, int64_t& a ) { a = ser_readdata64(s); } | |||
| template<typename Stream> inline void Unserialize(Stream& s, uint64_t& a) { a = ser_readdata64(s); } | |||
| template<typename Stream> inline void Unserialize(Stream& s, float& a ) { a = ser_uint32_to_float(ser_readdata32(s)); } | |||
| template<typename Stream> inline void Unserialize(Stream& s, float& a ) { static_assert(ALWAYS_FALSE<Stream>, "Not implemented"); } | |||
There was a problem hiding this comment.
Looks like the current templates are set up such that this fails compilation even if removed completely. Though, I am worried that in the future it will silently fall back from float -> double, so I've added the static_assert(false) to ensure this is never compiled.
There was a problem hiding this comment.
I kind of hope we can remove the double case as well some time soon. But I think this is fine for now.
There was a problem hiding this comment.
I'm fine with doing this or not. Even if it's accidentally invoked, at worst it'll result in serialization as a double, which is just 4 bytes bigger, but no loss compared to (highly hypothetical) expected float serialization.
There was a problem hiding this comment.
Should be easy to remove it in a follow-up. E.g #21966 (comment)
|
I think this can be closed as it is included in #21966. |
…ee estimation 66545da Remove support for double serialization (Pieter Wuille) fff1cae Convert uses of double-serialization to {En,De}codeDouble (Pieter Wuille) afd964d Convert existing float encoding tests (Pieter Wuille) bda33f9 Add unit tests for serfloat module (Pieter Wuille) 2be4cd9 Add platform-independent float encoder/decoder (Pieter Wuille) e40224d Remove unused float serialization (MarcoFalke) Pull request description: Based on #21981. This adds a software-based platform-independent float/double encoder/decoder (platform independent in the sense that it only uses arithmetic and library calls, but never inspects the binary representation). This should strengthen our guarantee that encoded float/double values are portable across platforms. It then removes the functionality to serialize doubles from serialize.h, and replaces its only (non-test) use for fee estimation data serialization with the software encoder. At least on x86/ARM, the only difference should be how certain NaN values are encoded/decoded (but not *whether* they are NaN or not). It comes with tests that verify on is_iec559 platforms (which are the only ones we support, at least for now) that the serialized bytes exactly match the binary representation of floats in memory (for non-NaN). ACKs for top commit: laanwj: Code review re-ACK 66545da practicalswift: cr re-ACK 66545da Tree-SHA512: 62ad9adc26e28707b2eb12a919feefd4fd10cf9032652dbb1ca1cc97638ac21de89e240858e80d293d5112685c623e58affa3d316a9783ff0e6d291977a141f5
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
|
Closing as #21966 was merged. |
…r for fee estimation 66545da Remove support for double serialization (Pieter Wuille) fff1cae Convert uses of double-serialization to {En,De}codeDouble (Pieter Wuille) afd964d Convert existing float encoding tests (Pieter Wuille) bda33f9 Add unit tests for serfloat module (Pieter Wuille) 2be4cd9 Add platform-independent float encoder/decoder (Pieter Wuille) e40224d Remove unused float serialization (MarcoFalke) Pull request description: Based on bitcoin#21981. This adds a software-based platform-independent float/double encoder/decoder (platform independent in the sense that it only uses arithmetic and library calls, but never inspects the binary representation). This should strengthen our guarantee that encoded float/double values are portable across platforms. It then removes the functionality to serialize doubles from serialize.h, and replaces its only (non-test) use for fee estimation data serialization with the software encoder. At least on x86/ARM, the only difference should be how certain NaN values are encoded/decoded (but not *whether* they are NaN or not). It comes with tests that verify on is_iec559 platforms (which are the only ones we support, at least for now) that the serialized bytes exactly match the binary representation of floats in memory (for non-NaN). ACKs for top commit: laanwj: Code review re-ACK 66545da practicalswift: cr re-ACK 66545da Tree-SHA512: 62ad9adc26e28707b2eb12a919feefd4fd10cf9032652dbb1ca1cc97638ac21de89e240858e80d293d5112685c623e58affa3d316a9783ff0e6d291977a141f5
No description provided.