Skip to content

[WIP] Fix: OSX QT compile fix: defer to bswap_XX when defined. #9361

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.

Sign up for GitHub

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

Jump to bottom
Closed
kallewoof wants to merge 3 commits into from
Closed

[WIP] Fix: OSX QT compile fix: defer to bswap_XX when defined. #9361

kallewoof wants to merge 3 commits into from

Conversation

@kallewoof
Copy link
Contributor

@kallewoof kallewoof commented Dec 16, 2016

Defer to (protobuf) defined bswap_XX macros when defined. This resolves compile errors for the QT client on OSX.

@kallewoof kallewoof changed the title Fix: OSX compile fix: defer to bswap_XX when defined. Fix: OSX QT compile fix: defer to bswap_XX when defined. Dec 16, 2016
jonasschnelli
jonasschnelli approved these changes Dec 16, 2016
View reviewed changes
Copy link
Contributor

@jonasschnelli jonasschnelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.
Code Review utACK e62650b5d5fa643daf337031cb4cb914704dd346

@laanwj
Copy link
Member

laanwj commented Dec 16, 2016

Yes, seems sensible. It's unfortunate that protobuf exports such common names as macros in their public interface.
The other way around would be to put #undef bswap_XX at the top. Don't have a clear preference though.

@kallewoof
Copy link
Contributor Author

kallewoof commented Dec 16, 2016

Protobuf detects and uses the OS defined OSSwapIntXX macros which might be a nice thing. Other than that yes I'm surprised they are as clumsy as they are considering it's Google.

@laanwj
Copy link
Member

laanwj commented Dec 16, 2016

Protobuf detects and uses the OS defined OSSwapIntXX macros which might be a nice thing

That would be a nice thing if you addded the same detection to our autoconf. The byteswap functions are mainly used in places that don't include (and can't depend on) protobuf.

Now this creates an inconsistency between compilation units that include protobuf and don't include protobuf, in unrelated code.

@kallewoof
Copy link
Contributor Author

kallewoof commented Dec 16, 2016 •
edited
Loading

That makes a lot of sense. I will dig.

@kallewoof kallewoof changed the title Fix: OSX QT compile fix: defer to bswap_XX when defined. [WIP] Fix: OSX QT compile fix: defer to bswap_XX when defined. Dec 16, 2016
@kallewoof
Uses built-in byte swap if available (Apple) and if bswap_XX is undef…
e83b11b 
…ined.

Defers to pre-defined version if found (e.g. protobuf). For protobuf case, the definitions are identical and thus include order should not affect results.
@kallewoof
Copy link
Contributor Author

kallewoof commented Dec 16, 2016

I somehow messed the rebase up. Reopening. Sorry about that.

@kallewoof kallewoof closed this Dec 16, 2016
@kallewoof kallewoof deleted the macosx-qt-build-fix branch December 16, 2016 12:30
@kallewoof kallewoof mentioned this pull request Dec 16, 2016
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@jonasschnelli jonasschnelli jonasschnelli approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kallewoof @laanwj @jonasschnelli @sipa