Add handling for MSGPACK_OBJECT_FLOAT{32,64} #6406
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
An alternative route here would be to replace the use of msgpack-c with libmpack, but we would still need to handle receiving float32 types from other users of the API. |
justinmk
reviewed
Mar 30, 2017
| @@ -973,7 +973,13 @@ int msgpack_to_vim(const msgpack_object mobj, typval_T *const rettv) | |||
| } | |||
| break; | |||
| } | |||
| case MSGPACK_OBJECT_FLOAT: { | |||
| #ifdef NVIM_MSGPACK_HAS_FLOAT32 | |||
There was a problem hiding this comment.
Can't we test MSGPACK_VERSION_* instead? https://github.com/msgpack/msgpack-c/blob/master/include/msgpack/version.h
That also avoids the extra cmake code.
There was a problem hiding this comment.
I generally prefer "feature" checks instead of version checks. It's not as relevant here, but they're more robust in terms of changes being backported.
justinmk
approved these changes
Mar 30, 2017
|
Looks like gcc on ubuntu 12 (quickbuild) doesn't build with this version of msgpack. |
msgpack-c previously only had MSGPACK_OBJECT_FLOAT, which was a 64-bit value. Now, 32-bit and 64-bit floats are supported as distinct types, but we'll simply continue to treat everything as 64-bit types.
jamessan
force-pushed
the
msgpack-c-2.1.x-compat
branch
from
922b6c4
to
193aa4c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
msgpack-c previously only had MSGPACK_OBJECT_FLOAT, which was a float64
value. Now, float32 and float64 are supported as distinct types,
but we'll simply continue to treat everything as float64.
There is an issue with using "nvim built against msgpack-c < 2.1.0" with
a msgpack-c >= 2.1.0 shared library, since nvim won't recognize a packed
MSGPACK_OBJECT_FLOAT32. I expect this to be handled by distributions (I
ran into it while trying to updated Debian's msgpack-c), but I still
wanted to raise the issue.
Cc @ZyX-I