pack double and float more size efficient #1018

GeorgFritze · 2022-05-13T11:25:42Z

PR for issue #1017

check also for nan and numeric limits

redboltz

LGTM. I commented some coding stype issue. Please fix them.

redboltz · 2022-06-03T11:22:56Z

include/msgpack/v1/pack.hpp

@@ -1138,6 +1138,18 @@ inline packer<Stream>& packer<Stream>::pack_unsigned_long_long(unsigned long lon
 template <typename Stream>
 inline packer<Stream>& packer<Stream>::pack_float(float d)
 {
+    if (d == d) { // check for nan 


Suggested change

if (d == d) { // check for nan

if(d == d) { // check for nan

redboltz · 2022-06-03T11:23:09Z

include/msgpack/v1/pack.hpp

@@ -1138,6 +1138,18 @@ inline packer<Stream>& packer<Stream>::pack_unsigned_long_long(unsigned long lon
 template <typename Stream>
 inline packer<Stream>& packer<Stream>::pack_float(float d)
 {
+    if (d == d) { // check for nan 
+        // compare d to limits::max() to avoid undefined behaviour
+        if (d >= 0 && d <= std::numeric_limits<uint64_t>::max() && d == uint64_t(d)) {


Suggested change

if (d >= 0 && d <= std::numeric_limits<uint64_t>::max() && d == uint64_t(d)) {

if(d >= 0 && d <= std::numeric_limits<uint64_t>::max() && d == uint64_t(d)) {

redboltz · 2022-06-03T11:24:12Z

include/msgpack/v1/pack.hpp

+        if (d >= 0 && d <= std::numeric_limits<uint64_t>::max() && d == uint64_t(d)) {
+            pack_imp_uint64(uint64_t(d));
+            return *this;
+


Please erase empty line above.

redboltz · 2022-06-03T11:24:23Z

include/msgpack/v1/pack.hpp

+            pack_imp_uint64(uint64_t(d));
+            return *this;
+
+        } else if (d >= std::numeric_limits<int64_t>::min() && d == int64_t(d)) {


Suggested change

} else if (d >= std::numeric_limits<int64_t>::min() && d == int64_t(d)) {

} else if(d >= std::numeric_limits<int64_t>::min() && d == int64_t(d)) {

redboltz · 2022-06-03T11:25:26Z

include/msgpack/v1/pack.hpp

+    if (d == d) { // check for nan 
+        // compare d to limits::max() to avoid undefined behaviour
+        if (d >= 0 && d <= std::numeric_limits<uint64_t>::max() && d == uint64_t(d)) {
+            pack_imp_uint64(uint64_t(d));
+            return *this;
+
+        } else if (d >= std::numeric_limits<int64_t>::min() && d == int64_t(d)) {
+            pack_imp_int64(int64_t(d));
+            return *this;
+        }
+    }
+


Please apply the same fix as float.

redboltz · 2022-06-05T09:35:38Z

It seems that CI reports warnings (as errors). Could you fix this?

/home/runner/work/msgpack-c/msgpack-c/include/msgpack/v1/pack.hpp:[114](https://github.com/msgpack/msgpack-c/runs/6744024570?check_suite_focus=true#step:8:115)3:27: error: implicit conversion from 'unsigned long' to 'float' may lose precision [-Werror,-Wimplicit-int-float-conversion]
        if(d >= 0 && d <= std::numeric_limits<uint64_t>::max() && d == uint64_t(d)) {
                       ~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/msgpack-c/msgpack-c/include/msgpack/v1/pack.hpp:1143:72: error: implicit conversion from 'uint64_t' (aka 'unsigned long') to 'float' may lose precision [-Werror,-Wimplicit-int-float-conversion]
        if(d >= 0 && d <= std::numeric_limits<uint64_t>::max() && d == uint64_t(d)) {
                                                                    ~~ ^~~~~~~~~~~
/home/runner/work/msgpack-c/msgpack-c/include/msgpack/v1/pack.hpp:1146:24: error: implicit conversion from 'long' to 'float' may lose precision [-Werror,-Wimplicit-int-float-conversion]
        } else if(d >= std::numeric_limits<int64_t>::min() && d == int64_t(d)) {
                    ~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/msgpack-c/msgpack-c/include/msgpack/v1/pack.hpp:1146:68: error: implicit conversion from 'int64_t' (aka 'long') to 'float' may lose precision [-Werror,-Wimplicit-int-float-conversion]
        } else if(d >= std::numeric_limits<int64_t>::min() && d == int64_t(d)) {
                                                                ~~ ^~~~~~~~~~
/home/runner/work/msgpack-c/msgpack-c/include/msgpack/v1/pack.hpp:[116](https://github.com/msgpack/msgpack-c/runs/6744024570?check_suite_focus=true#step:8:117)5:27: error: implicit conversion from 'unsigned long' to 'double' may lose precision [-Werror,-Wimplicit-int-float-conversion]
        if(d >= 0 && d <= std::numeric_limits<uint64_t>::max() && d == uint64_t(d)) {
                       ~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/msgpack-c/msgpack-c/include/msgpack/v1/pack.hpp:1165:72: error: implicit conversion from 'uint64_t' (aka 'unsigned long') to 'double' may lose precision [-Werror,-Wimplicit-int-float-conversion]
        if(d >= 0 && d <= std::numeric_limits<uint64_t>::max() && d == uint64_t(d)) {
                                                                    ~~ ^~~~~~~~~~~
/home/runner/work/msgpack-c/msgpack-c/include/msgpack/v1/pack.hpp:1168:24: error: implicit conversion from 'long' to 'double' may lose precision [-Werror,-Wimplicit-int-float-conversion]
        } else if(d >= std::numeric_limits<int64_t>::min() && d == int64_t(d)) {
                    ~~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/msgpack-c/msgpack-c/include/msgpack/v1/pack.hpp:1168:68: error: implicit conversion from 'int64_t' (aka 'long') to 'double' may lose precision [-Werror,-Wimplicit-int-float-conversion]
        } else if(d >= std::numeric_limits<int64_t>::min() && d == int64_t(d)) {

codecov-commenter · 2022-06-05T09:35:41Z

Codecov Report

Merging #1018 (05de839) into cpp_master (05f654f) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 05de839 differs from pull request most recent head 33ff3a4. Consider uploading reports for the commit 33ff3a4 to get more accurate results

@@              Coverage Diff               @@
##           cpp_master    #1018      +/-   ##
==============================================
+ Coverage       85.56%   85.60%   +0.04%     
==============================================
  Files              79       79              
  Lines            5009     5023      +14     
==============================================
+ Hits             4286     4300      +14     
  Misses            723      723

update pack_double and pack_float to avoid warnings

redboltz · 2022-06-13T14:02:35Z

Thank you for updating. Great work!

Floats may be stored as integers, and msgpack-cpp does that proactively since version 4.1.2, see msgpack/msgpack-c#1018

Floats may be stored as integers in msgpack, and msgpack-cpp does that proactively since version 4.1.2. That broke the `unitCell` angle round trip in `testSave_symmetry__mmtf`. See also: rcsb/mmtf-c#35 msgpack/msgpack-c#1018

This is v4.1.1 We can't update beyond v4.1.1 because of what is a breaking change for pdf2msgpack: msgpack/msgpack-c#1018

Floats may be stored as integers, and msgpack-cpp does that proactively since version 4.1.2, see msgpack/msgpack-c#1018

This reverts commit d13d933, reversing changes made to 05f654f.

Revert "Merge pull request #1018 from GeorgFritze/cpp_master"

This reverts commit d13d933, reversing changes made to 05f654f.

pack double and float more size efficient

6f0683b

GeorgFritze mentioned this pull request May 13, 2022

pack double and float in a more compact way #1017

Closed

GeorgFritze and others added 2 commits June 2, 2022 10:40

pack double and float in a more size efficient way

68acf21

check also for nan and numeric limits

Merge branch 'msgpack:cpp_master' into cpp_master

3b405fc

redboltz reviewed Jun 3, 2022

View reviewed changes

style changes

d5c837b

GeorgFritze added 3 commits June 7, 2022 09:46

update msgpack_basic test for compact double packaging

d59e1d7

update pack_double and pack_float to avoid warnings

fix bug in pack_float and pack_double

05de839

fix undefined bahaviour in msgpack_basic tests

33ff3a4

redboltz merged commit d13d933 into msgpack:cpp_master Jun 13, 2022

speleo3 added a commit to speleo3/mmtf-c that referenced this pull request Oct 6, 2022

Fix loading floats from packed integers

83c2f48

Floats may be stored as integers, and msgpack-cpp does that proactively since version 4.1.2, see msgpack/msgpack-c#1018

speleo3 mentioned this pull request Oct 6, 2022

Fix loading floats from packed integers rcsb/mmtf-c#35

Merged

speleo3 mentioned this pull request Oct 6, 2022

Fix testSave_symmetry__mmtf for msgpack-cpp=4.1.2 schrodinger/pymol-open-source#262

Merged

redboltz mentioned this pull request May 14, 2023

Packed and unpacked types are not the same when using msgpack::type::variant. #1070

Closed

StevenMaude mentioned this pull request Aug 4, 2023

Upgrade msgpack-cpp beyond v4.1.1 sensiblecodeio/pdf2msgpack#142

Closed

StevenMaude added a commit to sensiblecodeio/pdf2msgpack that referenced this pull request Aug 4, 2023

Update msgpack-c to 63511f2

32eca5e

This is v4.1.1 We can't update beyond v4.1.1 because of what is a breaking change for pdf2msgpack: msgpack/msgpack-c#1018

StevenMaude mentioned this pull request Aug 7, 2023

Upgrade to msgpack-cpp 6.1.1 sensiblecodeio/pdf2msgpack#146

Merged

speleo3 added a commit to rcsb/mmtf-c that referenced this pull request Sep 26, 2023

Fix loading floats from packed integers

b12bb3a

Floats may be stored as integers, and msgpack-cpp does that proactively since version 4.1.2, see msgpack/msgpack-c#1018

romankarlstetter mentioned this pull request Mar 22, 2024

Cannot serialize small floats in msgpack float format #1111

Open

redboltz mentioned this pull request Aug 22, 2024

Conversions between float and int are not type-checked. #1139

Open

stephanlachnit added a commit to stephanlachnit/msgpack-c that referenced this pull request Oct 24, 2024

Revert "Merge pull request msgpack#1018 from GeorgFritze/cpp_master"

3788b5b

This reverts commit d13d933, reversing changes made to 05f654f.

stephanlachnit mentioned this pull request Oct 24, 2024

Revert "Merge pull request #1018 from GeorgFritze/cpp_master" #1144

Merged

redboltz added a commit that referenced this pull request Oct 31, 2024

Merge pull request #1144 from stephanlachnit/p-cpp-revert-broken-float

405977d

Revert "Merge pull request #1018 from GeorgFritze/cpp_master"

jiahansu pushed a commit to WiseSync/msgpack-c that referenced this pull request Jan 9, 2025

Revert "Merge pull request msgpack#1018 from GeorgFritze/cpp_master"

c4cfb3f

This reverts commit d13d933, reversing changes made to 05f654f.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pack double and float more size efficient #1018

pack double and float more size efficient #1018

GeorgFritze commented May 13, 2022 •

edited

Loading

redboltz left a comment

redboltz Jun 3, 2022

redboltz Jun 3, 2022

redboltz Jun 3, 2022

redboltz Jun 3, 2022

redboltz Jun 3, 2022

redboltz commented Jun 5, 2022 •

edited

Loading

codecov-commenter commented Jun 5, 2022 •

edited

Loading

redboltz commented Jun 13, 2022

	if (d >= 0 && d <= std::numeric_limits<uint64_t>::max() && d == uint64_t(d)) {
	if(d >= 0 && d <= std::numeric_limits<uint64_t>::max() && d == uint64_t(d)) {

	} else if (d >= std::numeric_limits<int64_t>::min() && d == int64_t(d)) {
	} else if(d >= std::numeric_limits<int64_t>::min() && d == int64_t(d)) {

pack double and float more size efficient #1018

pack double and float more size efficient #1018

Conversation

GeorgFritze commented May 13, 2022 • edited Loading

redboltz left a comment

Choose a reason for hiding this comment

redboltz Jun 3, 2022

Choose a reason for hiding this comment

redboltz Jun 3, 2022

Choose a reason for hiding this comment

redboltz Jun 3, 2022

Choose a reason for hiding this comment

redboltz Jun 3, 2022

Choose a reason for hiding this comment

redboltz Jun 3, 2022

Choose a reason for hiding this comment

redboltz commented Jun 5, 2022 • edited Loading

codecov-commenter commented Jun 5, 2022 • edited Loading

Codecov Report

redboltz commented Jun 13, 2022

GeorgFritze commented May 13, 2022 •

edited

Loading

redboltz commented Jun 5, 2022 •

edited

Loading

codecov-commenter commented Jun 5, 2022 •

edited

Loading