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

Fix compilation under OS X together with Core Foundation headers #406

Closed
wants to merge 1 commit into from

Conversation

vadz
Copy link

@vadz vadz commented Jan 17, 2016

/usr/include/MacTypes.h included by many (all?) Core Foundation headers
defines "nil" as "NULL" breaking compilation of msgpack code. Undo this
definition in the header included before all the other ones to allow using
msgpack and Core Foundation together.

/usr/include/MacTypes.h included by many (all?) Core Foundation headers
defines "nil" as "NULL" breaking compilation of msgpack code. Undo this
definition in the header included before all the other ones to allow using
msgpack and Core Foundation together.
@redboltz
Copy link
Contributor

@vadz, thank you for sending the PR. The same problem has been reported. See #368.
I think that if you undef nil, then harmful side effects might occur. Because client codes that include msgpack.hpp might expect nil as NULL.

I will take over #368. That means replace nil with nil_t. And provides the following typedef:

#if MSGPACK_USE_LEGACY_NIL // not defined by default
typedef nil_t nil;
#endif

@vadz, is that OK?

@vadz
Copy link
Author

vadz commented Jan 18, 2016

Thanks for looking at this and sorry for not finding #368 myself (BTW, I'm using "plain" C++, not Objective-C++, so the problem is not limited to ObjC code).

I agree that renaming nil to nil_t is a better approach, I just wanted to make the minimally intrusive (and maximally backwards-compatible) fix, but if you think that msgpack::types::nil is not widely used, renaming it would make more sense.

Thanks again!

@redboltz
Copy link
Contributor

The problem has been fixed by #408. So I close the PR.

@redboltz redboltz closed this Jan 20, 2016
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