Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

integer API cleanup #70

Open
jrahlf opened this issue May 2, 2022 · 1 comment
Open

integer API cleanup #70

jrahlf opened this issue May 2, 2022 · 1 comment

Comments

@jrahlf
Copy link

jrahlf commented May 2, 2022

The read/write functions are not symmetric and confusing:

bool cmp_write_integer(cmp_ctx_t *ctx, int64_t d);

bool cmp_read_int(cmp_ctx_t *ctx, int32_t *i);
bool cmp_read_long(cmp_ctx_t *ctx, int64_t *u);
bool cmp_read_integer(cmp_ctx_t *ctx, int64_t *u);

I would suggest the following:

  • remove int/integer duplicate functions
  • uses distinct names for 32-bit and 64-bit types: int/long or maybe int32/int64
  • add dedicated 32-bit write function (read function is already there) -> this is especially advantageous for 32-bit systems, e.g. microcontrollers, where 64-bit ints are emulated in software
bool cmp_write_int(cmp_ctx_t *ctx, int32_t d);
bool cmp_write_long(cmp_ctx_t *ctx, int64_t d);

bool cmp_read_int(cmp_ctx_t *ctx, int32_t *i);
bool cmp_read_long(cmp_ctx_t *ctx, int64_t *d);
@camgunz
Copy link
Owner

camgunz commented May 17, 2022

Hiya, thanks for filing this!

The MessagePack spec contains this little tidbit:

If an object can be represented in multiple possible output formats, serializers SHOULD use the format which represents the data in the smallest number of bytes.

CMP has a few APIs, but its main API maps to MessagePack's high level types and implements the above. If we added symmetric write methods we'd still have to switch on the value and write the appropriate type marker. That plus C integer promotion makes them pretty superfluous.

Buuuuut you make a good point about non-64-bit architectures. I think there's probably a feature in there for that, to define away support for integers that aren't supported in hardware. I think I'll file a feature for it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants