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 compatibility with -Wshadow and ObjC++ #368

Closed
wants to merge 1 commit into from

Conversation

darknoon
Copy link

@darknoon darknoon commented Oct 1, 2015

Hi, we have a project that uses ObjC++ and msgpack-c. Therefore, we can't use nil as the name of a type since it's a preprocessor macro in ObjC that resolves to 0. This PR changes the name to nil_t for compatibility and hopefully adds a new audience for msgpack-c.

Additionally, we compile with -Wshadow in clang, which doesn't like

with_zone(msgpack::zone& zone) : zone(zone) { }

What do you think? I'm happy to split into two PRs if that would be better for you.

Thanks!

@redboltz
Copy link
Contributor

redboltz commented Oct 2, 2015

Hi @darknoon, thank you for sending the PR.

Replacing nil with nil_t could break existing code. We need to consider that. We usually apply that kind of breaking change at major version up.

There are some approaches I just came up with.

#if objcpp // somehow detect objc++
struct nil_t { };
#else
struct nil { };
#endif

It seems that we need to write those macro many times. It's not good.

struct nil_t { }; // replace all nil with nil_t
#if !objcpp // NOT objcpp
typedef nil_t nil;
#endif

I haven't tested yet but I believe that it would work well.

After major version up, nil_t is a default. In addition providing MSGPACK_USE_LEGACY_NIL to support older code.

struct nil_t { }; // replace all nil with nil_t
#if MSGPACK_USE_LEGACY_NIL // not defined by default
typedef nil_t nil;
#endif

@nobu-k, what do you think?

Additionally, we compile with -Wshadow in clang, which doesn't like

with_zone(msgpack::zone& zone) : zone(zone) { }

I agree. I think that this part of the PR should be merged.

What do you think? I'm happy to split into two PRs if that would be better for you.

Splitting the PR is nice :) Could you split it?

@nobu-k
Copy link
Member

nobu-k commented Oct 3, 2015

struct nil_t { }; // replace all nil with nil_t
#if !objcpp // NOT objcpp
typedef nil_t nil;
#endif

I think this approach is sufficient. I'm not sure if we should provide MSGPACK_USE_LEGACY_NIL at the next major version up because nil might already be used in many projects and having both nil and nil_t isn't too harmful. It's better, though, if we can announce nil is deprecated in some way.

@redboltz
Copy link
Contributor

redboltz commented Oct 6, 2015

@nobu-k, thank you for the comment. Basically I agree with you.

The first step is:
0. Split the PR.

  1. Replace all nil with nil_t. (Including tests.)
  2. Add typedef nil_t nil if compiling environment is objc++ ( I don't know how to detect it.)

@darknoon, could you do that?

@redboltz
Copy link
Contributor

The PR is separated as #408 #409 and both merged. So I close the PR.

@redboltz redboltz closed this Jan 20, 2016
@darknoon
Copy link
Author

darknoon commented Feb 3, 2016

Thanks @redboltz, I just missed the notification on your comment earlier. I appreciate your picking it up and fixing.

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