Skip to content

Conversation

@redboltz
Copy link
Contributor

Introduced new object type FLOAT32 and FLOAT64.
FLOAT64 is equivalent to FLOAT.
The both internal expressions are f64(double).

Introduced new object type `FLOAT32` and `FLOAT64`.
`FLOAT64` is equivalent to `FLOAT`.
The both internal expressions are f64(double).
@redboltz
Copy link
Contributor Author

This PR will fix #521. It is a breaking change but very subtle.

  • Introduced the new msgpack::type::FLOAT32.
  • Introduced the name alias msgpack::type::FLOAT64 as existing msgpack::type::FLOAT.
  • Internal expression (representation) of msgpack::object isn't changed.
situation example before after
create msgpack::object from float msgpack::object(1.2F) FLOAT FLOAT32
unpack float32 to msgpack::object 0xca, 0x00, 0x00, 0x00, 0x00 FLOAT FLOAT32
pack msgpack::object type FLOAT32 msgpack::object obj(0.0F); msgpack::pack(stream, obj); 0xcb, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 0xca, 0x00, 0x00, 0x00, 0x00

I guess that all cases do NOT make big impact for existing client codes.

When creating and packing, existing client codes only use FLOAT that is equivalent to FLOAT64. So there is no impact.

When unpacking, if a client code compare the msgpack::object::type to FLOAT, it fails only when msgpack::object is unpacked from float32.

However, it is easy to migrate as follows:

Replace

if (obj.type == msgpack::type::FLOAT) {
}

with

if (obj.type == msgpack::type::FLOAT32 || obj.type == msgpack::type::FLOAT64) {
}

After applying the PR, FLOAT still exists so you can also write as follows:

if (obj.type == msgpack::type::FLOAT32 || obj.type == msgpack::type::FLOAT) {
}

I recommend to use FLOAT64 instead of FLOAT. Because it is easy to understand.

If you convert using convert() or as(), there is no change. Both FLOAT32 and FLOAT64 can covert to any combination of float and double

@mika-fischer , is this satisfied you usecase?

I think that it is a breaking change but doesn't make a big impact. So I don't think the major version up is required e.g.) 3.0.0. I think that we can release it as 2.1.0.

@nobu-k, what do you think?

@mika-fischer
Copy link
Contributor

@redboltz Wow, thanks, looks good!

@redboltz
Copy link
Contributor Author

@nobu-k, what do you think? I will merge it to master. I'd like to hear your idea of backward compatibility.

@nobu-k
Copy link
Member

nobu-k commented Oct 28, 2016

I think this change does have a big impact on users who have already used msgpack::type::FLOAT in their code. However,

  • Already compiled programs still work correctly
  • All existing projects can still be built without a compile error
  • The problem only occurs when a user explictly checks msgpack::type::FLOAT AND tries to unpack float32.

So, if we want to release this as v2.1.0, we have to make a clear announcement about the change and provide a migration guide you wrote above.

By the way, is there any way to produce a warning at the compile time if msgpack::type::FLOAT is used?

@redboltz
Copy link
Contributor Author

By the way, is there any way to produce a warning at the compile time if msgpack::type::FLOAT is used?

since C++14, we can do that as follows:
http://melpon.org/wandbox/permlink/4Kew24FostSXdoG0

@redboltz
Copy link
Contributor Author

For old compilers, we can introduce the macro MSGPACK_DEPRECATED.

http://melpon.org/wandbox/permlink/TlKE5kdGVND2W5xj

#if __cplusplus >= 201402L 
#define MSGPACK_DEPRECATED [[deprecated]]
#else
#define MSGPACK_DEPRECATED
#endif

enum {
    A,
    B MSGPACK_DEPRECATED,
    C
};

int main() {
    int a = A;
    int b = B;
    int c = C;

    (void)a;
    (void)b;
    (void)c;
}

@nobu-k
Copy link
Member

nobu-k commented Oct 29, 2016

Looks great!

@redboltz
Copy link
Contributor Author

I will merge the PR without deprecated marking. After I do it, I will write the new PR for deprecated marking. I created the issue for that #534

The reason that I separate the deprecated marking from the PR, there are some places that should be marked as deprecated, not only the PR.

@redboltz redboltz merged commit 401460b into msgpack:master Oct 29, 2016
@redboltz redboltz deleted the fix_521 branch October 29, 2016 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants