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

Preliminary implementation of the formatted packer #416

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

HalosGhost
Copy link

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.

@redboltz
Copy link
Contributor

redboltz commented Feb 6, 2016

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?

@HalosGhost
Copy link
Author

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?

@redboltz
Copy link
Contributor

redboltz commented Feb 7, 2016

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 :)

@HalosGhost
Copy link
Author

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!

@HalosGhost HalosGhost changed the title First run of the generic integer packing function Preliminary implementation of the formatted packer Feb 7, 2016
@HalosGhost
Copy link
Author

@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 packf() function. However, it seems the test suite is all in C++. C11's _Generic keyword is not present in C++. How can I accomplish testing these?

@redboltz
Copy link
Contributor

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 ;)

@HalosGhost
Copy link
Author

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.

@HalosGhost
Copy link
Author

@redboltz, I have been looking into this for a while and the best thing I've found so far is Check. Does that seem amenable to you? If so, then I will go ahead and see if I can integrate it into the current build-system without wreaking havoc.

@redboltz
Copy link
Contributor

It looks good to me. Check's license is LGPL. The files for tests contains Check's header file, but it is not a part of the distribution package similar to the googletest.

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 Check. Both the googletest and the Check should be kicked by make check (autotools) and make test (cmake).

@HalosGhost
Copy link
Author

@redboltz, sounds good to me. I will take a look at implementing the checks as soon as I have a chance.

@ChisholmKyle
Copy link

For unit testing C, I strongly recommend https://github.com/ThrowTheSwitch/Unity
You just need to add unity source file to your test targets - no need for a library like check!

@ChisholmKyle
Copy link

I just realized this thread a few years old... feel free to disregard my comments

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.

4 participants