Remove unused float serialization #21981

1. 
   MarcoFalke commented at 11:10 am on May 17, 2021: member
2. Remove unused float serialization fa57a8fed1
3. laanwj added the label Utils/log/libs on May 17, 2021
4. 
   laanwj commented at 11:23 am on May 17, 2021: member

   Good catch!

   Code review ACK fa57a8fed196c5e89659265e1b39efb0e2bec9d0

5. 
   practicalswift commented at 1:59 pm on May 17, 2021: contributor

   Nice!

   cr ACK fa57a8fed196c5e89659265e1b39efb0e2bec9d0

6. 
   sipa commented at 3:29 pm on May 17, 2021: member

   Concept ACK, certainly no need to keep this longer if it’s unused.
7. 
   DrahtBot commented at 3:37 pm on May 17, 2021: member

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #21966 (Remove double serialization; use software encoder for fee estimation by sipa)

   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.

8. in src/serialize.h:242 in fa57a8fed1

   238@@ -252,7 +239,7 @@ template<typename Stream> inline void Unserialize(Stream& s, int32_t& a ) { a =
   239 template<typename Stream> inline void Unserialize(Stream& s, uint32_t& a) { a = ser_readdata32(s); }
   240 template<typename Stream> inline void Unserialize(Stream& s, int64_t& a ) { a = ser_readdata64(s); }
   241 template<typename Stream> inline void Unserialize(Stream& s, uint64_t& a) { a = ser_readdata64(s); }
   242-template<typename Stream> inline void Unserialize(Stream& s, float& a   ) { a = ser_uint32_to_float(ser_readdata32(s)); }
   243+template<typename Stream> inline void Unserialize(Stream& s, float& a   ) { static_assert(ALWAYS_FALSE<Stream>, "Not implemented"); }
   

   ---

   ---

   

   MarcoFalke commented at 3:37 pm on May 17, 2021:

   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.

   ---

   

   laanwj commented at 6:04 pm on May 17, 2021:

   I kind of hope we can remove the double case as well some time soon. But I think this is fine for now.

   ---

   

   sipa commented at 6:06 pm on May 17, 2021:

   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.

   ---

   

   MarcoFalke commented at 5:30 am on May 18, 2021:

   Should be easy to remove it in a follow-up. E.g #21966 (comment)
9. 
   laanwj commented at 8:43 am on May 19, 2021: member

   I think this can be closed as it is included in #21966.
10. laanwj referenced this in commit 707ba8692b on May 26, 2021
11. DrahtBot added the label Needs rebase on May 26, 2021
12. 
    DrahtBot commented at 9:55 am on May 26, 2021: member

    🐙 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”.

13. 
    laanwj commented at 9:30 am on May 27, 2021: member

    Closing as #21966 was merged.
14. laanwj closed this on May 27, 2021

    ---

15. sidhujag referenced this in commit efa10235fa on May 27, 2021
16. fanquake removed the label Needs rebase on May 31, 2021
17. MarcoFalke deleted the branch on May 31, 2021
18. DrahtBot locked this on Aug 18, 2022

Contributors
MarcoFalke laanwj practicalswift sipa DrahtBot

Labels
Utils/log/libs