pack double and float more size efficient by GeorgFritze · Pull Request #1018 · msgpack/msgpack-c

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

 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

 {
+ 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

+ 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

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


Suggested change
} elseif(d >= std::numeric_limits<int64_t>::min() && d == int64_t(d)) {
} elseif(d >= std::numeric_limits<int64_t>::min() && d == int64_t(d)) {

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

+ 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

GeorgFritzeand 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_masterJun 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 cantabular/pdf2msgpack#142
Closed

StevenMaude added a commit to cantabular/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 cantabular/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 requestmsgpack#1018from 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#1144from 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 requestmsgpack#1018from GeorgFritze/cpp_master"
c4cfb3f
This reverts commit d13d933, reversing changes made to 05f654f.

Navigation Menu

Uh oh!

pack double and float more size efficient - #1018

pack double and float more size efficient#1018
redboltz merged 7 commits into
msgpack:cpp_masterfrom
GeorgFritze:cpp_master

GeorgFritze commented May 13, 2022•
edited
Loading

Uh oh!

redboltz left a comment

Uh oh!

redboltzJun 3, 2022

Uh oh!

redboltzJun 3, 2022

Uh oh!

redboltzJun 3, 2022

Uh oh!

redboltzJun 3, 2022

Uh oh!

redboltzJun 3, 2022

Uh oh!

redboltz commented Jun 5, 2022•
edited
Loading

Uh oh!

codecov-commenter commented Jun 5, 2022•
edited
Loading

Uh oh!

redboltz commented Jun 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	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)) {

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

Uh oh!

Conversation

GeorgFritze commented May 13, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

redboltz left a comment

Choose a reason for hiding this comment

Uh oh!

redboltzJun 3, 2022

Choose a reason for hiding this comment

Uh oh!

redboltzJun 3, 2022

Choose a reason for hiding this comment

Uh oh!

redboltzJun 3, 2022

Choose a reason for hiding this comment

Uh oh!

redboltzJun 3, 2022

Choose a reason for hiding this comment

Uh oh!

redboltzJun 3, 2022

Choose a reason for hiding this comment

Uh oh!

redboltz commented Jun 5, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jun 5, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

redboltz commented Jun 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GeorgFritze commented May 13, 2022•
edited
Loading

redboltz commented Jun 5, 2022•
edited
Loading

codecov-commenter commented Jun 5, 2022•
edited
Loading