-
Notifications
You must be signed in to change notification settings - Fork 883
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
Preliminary implementation of the formatted packer #416
base: master
Are you sure you want to change the base?
Conversation
Hi @HalosGhost, thank you for sending the PR. It seems that the code is for C11. Could you tell me you have any plan to support C99 or older one? |
Well, the schema I had planned for the initial run is so much simpler to implement in C11 (mainly because it lets us dispatch on type). As a result, I had really only planned to support C11 right away. I can do C99, but the specifier scheme will be far more complex (we will need a separate specifier or a modifier for each of the integer types and for both floats). In fact, because we would probably want to keep backwards-compatibility, if we support C99 at all, it would radically increase the amount of work for this. Would it be a requirement for merge to have C99- support? |
No. I just wanted to know your plan. Please proceed the step. I think that it is better to merge after finishing implementation including tests. You can do additional commit and push the branch that corresponding to the pull request. I can review that step by step. The reason that I don't want to merge step by step is due to unpredictable issues, design could be change in the future, and then I can't keep commit logs clean. I look forward to the next commit :) |
Oh, that's perfectly understandable! Yeah, if it is not going to be a problem, I would definitely prefer to do this in C11 for now. Thanks for the heads-up and I'll keep pushing forward! |
@redboltz, I have a question. I would like to add tests for these two dispatcher macros before I begin getting into the meat of the |
I don't know much about C11. msgpack-c uses google test. Unfortunately, it seems not to be supported C11. I think that a C11 testing framework is required. I searched "C11 testing framework", and just glance the results, but I didn't get useful information. If you know a good testing framework, please introduce it. This PR is a C11 code for the first time in the msgpack-c. We might need to overcome many problems ;) |
Haha, fair enough. I am not terribly familiar with writing tests for my C code as I tend to just take a scorched-earth approach to error-handling so that 100 percent of cases (outside of undefined behavior) are handled correctly (or, at least, in a predictable fashion). I completely recognize the importance of tests for a large project though and would love to contribute tests for all the code that I write. I will take a look at what good test suites are available for C11-proper and will let you know what I find so we can move forward. |
It looks good to me. We already have tests for the C part of msgpack-c that use googletest. I think that the first step is that the tests for formatted packer only use the |
@redboltz, sounds good to me. I will take a look at implementing the checks as soon as I have a chance. |
For unit testing C, I strongly recommend https://github.com/ThrowTheSwitch/Unity |
I just realized this thread a few years old... feel free to disregard my comments |
This is the first bit of sugar I would like to add to msgpack-c.
Eventually, this will be used by my implementation of
msgpack_packf()
as discussed in #393.Note: this is incredibly preliminary.