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

Fixed build for mixed C/C++ configurations. #96

Closed
wants to merge 1 commit into from

Conversation

jrlanglois
Copy link
Contributor

No description provided.

@dbry
Copy link
Owner

dbry commented Feb 28, 2021

We actually had this change in a long time ago (from you) and I had to revert it because it broke big-endian builds (which makes those unsigned char pointers). See this commit.

I realize that we could get around that issue with some more #ifdef's, but the whole purpose of doing it that way was to make the code simple. It seems to me that since this is C code, there's no good reason to jump through hoops make it C++ compatible. Perhaps building libwavpack as a stand-alone library would be the right way to go in your project?

@jrlanglois
Copy link
Contributor Author

jrlanglois commented Mar 1, 2021

Perhaps building libwavpack as a stand-alone library would be the right way to go in your project?

No, no it wouldn't. I come from a modern school of thought of deliberately avoiding static libs, dynamic libs, and the subsequent overhead with maintaining all of that in a cross-platform environment. In fact, we surround ourselves with unity build modules so as to make dragging and dropping third party libraries into projects, enabling them on a whim, disabling them on a whim, and so on, a much easier task. Till C++ gets a package manager, this is the easiest and most modern way we have for our teams, one that creates the smallest binaries and fastest build speeds out of our compilers and linkers.

I hear your fight of leaving the C code be pure C code, and do sympathise with the overhead of managing a library. It seems to me that I'm better off sticking to my own fork, modifying it to suit our needs.

For some details; #35

@jrlanglois jrlanglois closed this Mar 1, 2021
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