Skip to content

Added target_arch for repr #26

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

Merged
merged 1 commit into from
Jul 9, 2014
Merged

Conversation

davbo
Copy link
Contributor

@davbo davbo commented Jul 9, 2014

c_ulong is u32 on x86

Haven't played with target_arch before but I'm guessing this will continue to work for x86_64 people..

@sfackler
Copy link
Owner

sfackler commented Jul 9, 2014

This will break non-x86/x86-64 platforms. I think the right way to go here is to have an enum with an unspecified representation with methods to map to and from libc::c_ulong.

@sfackler
Copy link
Owner

sfackler commented Jul 9, 2014

Actually, that enum isn't used anywhere, and can probably just be deleted until someone actually implements that chunk of the API.

@davbo
Copy link
Contributor Author

davbo commented Jul 9, 2014

Ah, here was me just about to do as you suggested! Shall I remove it in the pull request?

@sfackler
Copy link
Owner

sfackler commented Jul 9, 2014

Sure

@davbo
Copy link
Contributor Author

davbo commented Jul 9, 2014

Done, not sure if a comment needs to be left as I'm not familiar with X.509 so I'm probably not the best person to leave one anyway..

@sfackler
Copy link
Owner

sfackler commented Jul 9, 2014

Can you squash the commits together? LGTM other than that, thanks!

To be added at a later date when the API is expanded
@davbo
Copy link
Contributor Author

davbo commented Jul 9, 2014

Sure, done

sfackler added a commit that referenced this pull request Jul 9, 2014
@sfackler sfackler merged commit f3b2155 into sfackler:master Jul 9, 2014
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.

2 participants