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

add more primitive types support #569

Merged
merged 4 commits into from Mar 28, 2017

Conversation

Projects
None yet
3 participants
@ahui2823
Contributor

ahui2823 commented Mar 16, 2017

support more like unsigned int , unsigned short etc...

@billinghamj

This comment has been minimized.

Show comment
Hide comment
@billinghamj

billinghamj Mar 16, 2017

Member

Thanks! I'd like to get this merged ASAP, but I do need you to add reasonably extensive unit tests covering the use of each one of these types. This is very important to ensure no compatibility issues or regressions arise in the future.

Member

billinghamj commented Mar 16, 2017

Thanks! I'd like to get this merged ASAP, but I do need you to add reasonably extensive unit tests covering the use of each one of these types. This is very important to ensure no compatibility issues or regressions arise in the future.

@ahui2823

This comment has been minimized.

Show comment
Hide comment
@ahui2823

ahui2823 Mar 20, 2017

Contributor

add Unit tests for each type, and fix char convert to BOOL issues, this is unexpected.

Contributor

ahui2823 commented Mar 20, 2017

add Unit tests for each type, and fix char convert to BOOL issues, this is unexpected.

@billinghamj

This comment has been minimized.

Show comment
Hide comment
@billinghamj

billinghamj Mar 20, 2017

Member

Thanks! The commented code should be removed if no longer needed though.

It might also be worth doing a few extra checks around the cases that the commented code handled, as I'm not quite sure what it was meant to be doing.

Member

billinghamj commented Mar 20, 2017

Thanks! The commented code should be removed if no longer needed though.

It might also be worth doing a few extra checks around the cases that the commented code handled, as I'm not quite sure what it was meant to be doing.

@billinghamj

This comment has been minimized.

Show comment
Hide comment
@billinghamj

billinghamj Mar 24, 2017

Member

@ahui2823 If you can sort out the couple of things I mentioned above, I can get this merged

Member

billinghamj commented Mar 24, 2017

@ahui2823 If you can sort out the couple of things I mentioned above, I can get this merged

@ahui2823

This comment has been minimized.

Show comment
Hide comment
@ahui2823

ahui2823 Mar 27, 2017

Contributor

Previous implementation is incorrect, it consider char as a BOOL structure. The property attributes of BOOL has a prefix TB, and the property attributes of char has a prefix Tc.

Contributor

ahui2823 commented Mar 27, 2017

Previous implementation is incorrect, it consider char as a BOOL structure. The property attributes of BOOL has a prefix TB, and the property attributes of char has a prefix Tc.

@billinghamj billinghamj merged commit 9db4013 into jsonmodel:master Mar 28, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@billinghamj

This comment has been minimized.

Show comment
Hide comment
@billinghamj
Member

billinghamj commented Mar 28, 2017

Thanks @ahui2823 !

@billinghamj

This comment has been minimized.

Show comment
Hide comment
@billinghamj

billinghamj Apr 26, 2017

Member

@ahui2823 Looks like this change conflicts with #581 for 5th gen iPod Touch devices. Could you take a look at the other PR?

Member

billinghamj commented Apr 26, 2017

@ahui2823 Looks like this change conflicts with #581 for 5th gen iPod Touch devices. Could you take a look at the other PR?

@billinghamj

This comment has been minimized.

Show comment
Hide comment
@billinghamj

billinghamj May 4, 2017

Member

@ahui2823 Okay I think we're probably going to be rolling back this change. It seems to be causing too many issues.

Member

billinghamj commented May 4, 2017

@ahui2823 Okay I think we're probably going to be rolling back this change. It seems to be causing too many issues.

@mstarke

This comment has been minimized.

Show comment
Hide comment
@mstarke

mstarke Oct 12, 2017

This merge seems to cause crashes on macOS systems #601 other issues #585

mstarke commented Oct 12, 2017

This merge seems to cause crashes on macOS systems #601 other issues #585

@mstarke

This comment has been minimized.

Show comment
Hide comment
@mstarke

mstarke Oct 12, 2017

A test under macOS 10.12 with 10.13 SDK yield that BOOL properties are reported as Tc ergo char

mstarke commented Oct 12, 2017

A test under macOS 10.12 with 10.13 SDK yield that BOOL properties are reported as Tc ergo char

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