Closed etodanik closed 3 years ago
Thanks for the PR. The last commit to replace UINT32_MAX
with SIZE_MAX
isn't correct though. MessagePack doesn't support 64-bit lengths so UINT32_MAX
is the limit for str/bin/ext. In all three cases the line right after these checks casts it to uin32_t
which is why it's important that the check is against UINT32_MAX
. With the change to SIZE_MAX
, this cast can now overflow.
Also, I mentioned in #74 that I'm a bit uncomfortable with the double to float conversion code being copied from ArduinoJson without attribution. ArduinoJson appears to also be MIT-licensed so it's not much of an issue but I'll probably add at least a link to it.
@ethanjli Are you OK with your commits to support MPACK_DOUBLES
being merged into this repo?
@ludocode the problem is with platforms where UINT32_MAX is much larger than SIZE_MAX (e.g AVR where size_t is 16 bit). My code wouldn't compile without this change. Knowing what you explained, I can think of another change that will check the proper size of size_t but not allow it to be bigger than 32 bit.
I can also add attribution.
Would those changes make the code correct enough to be accepted here as a PR?
Thanks for making the PR - I haven't had time to look at this in the past several weeks, but I'm OK with my commits being merged into this repo.
I believe this is now all fixed.
I merged up to a2fe1b7 from this PR but renamed MPACK_DOUBLES
to MPACK_DOUBLE
.
I added an MPACK_FLOAT
configuration to also disable float
and expanded them a lot to encompass anything with float
or double
keywords. (These keywords are now poisoned within MPack and its unit test suite when disabled to ensure they are not used.)
I left out 9a8872c and instead added some "raw" functions for dealing with MessagePack floats as uint32_t
and doubles as uint64_t
.
I left out f8d15a1 and instead wrote my own code to manually convert doubles to floats. It's not quite as precise as a proper hardware float conversion but it should handle NAN and infinity correctly without the weird gotchas in the link in #74.
I cherry-picked 5352bb4 and fixed it up so that both SIZE_MAX
and UINT32_MAX
are respected.
I made various other fixes and improvements for AVR. For example MPACK_STDLIB
, MPACK_STDIO
, MPACK_STRINGS
and MPACK_DOUBLE
are all disabled automatically on AVR, and some issues with 16-bit int are fixed.
I added a Makefile to build the unit test suite with avr-gcc
.
The unit tests don't actually work on AVR yet. For one thing the unit test suite is way too big so it doesn't link, and for another, I don't have the entry point set up properly or something because e.g. -flto
strips everything. But it does at least compile all the source files without errors or warnings. I don't actually have an Arduino or any other microcontroller to test it (and I don't have time to get simavr set up) so I can't run any of this at the moment but hopefully at least MPack works.
This PR integrates the fixes by @ethanjli as well as a small fix by me to prevent an overflow error