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 some compilation errors on different platforms #64

Closed
wants to merge 2 commits into from

Conversation

othiman
Copy link
Contributor

@othiman othiman commented Nov 9, 2017

Hi.
I fixed some compilation errors on Windows and Linux and added a .gitignore file to unclutter the git status command. This should at least fix #63.

I would be happy if you could include these patches into the dev branch.

Best regards,
Thomas

@dennisguse
Copy link
Member

Cool!
It would be great if you could split up the pull request into two separate one: one for .gitignore and one for the build toolchain.
It is easier to discuss then ;)

Two minor thing:

  • Almost everywhere in the code base ´#ifdef defined(unix) || defined(__unix)´ is used.
    Could you change this in the patch?
  • Are the g711, pshar, and rpelt working without unistd.h?

@dennisguse
Copy link
Member

I will work on this pull request in October.

- Linux already has a getopt() function via unistd.h
@othiman othiman changed the title fix some compilation errors on different platforms and add a useful .gitignore file fix some compilation errors on different platforms Sep 26, 2018
@othiman
Copy link
Contributor Author

othiman commented Sep 26, 2018

Hi. I cherrypicked the PR and now only the compilation fixes are included.

@othiman
Copy link
Contributor Author

othiman commented Sep 26, 2018

Two minor thing:

* Almost everywhere in the code base ´#ifdef defined(**unix**) || defined(__unix)´ is used.
  Could you change this in the patch?

Where did you find the above line? I grepped the code for "unix" and most of the time "#if defined (unix)" is used. Maybe we should agree on some definition and then change it everywhere in the code to the same.

* Are the g711, pshar, and rpelt working without `unistd.h`?

I do not know. I will try out when I find some time for that.

@dennisguse
Copy link
Member

dennisguse commented Sep 26, 2018

@othiman
My fault.
It is actually: #ifdef defined(unix) || defined(__unix)
I added the asterisks as bold formatting.
In addition, the checks are at the moment not done everywhere consistently.
Sometimes, it is only defined(unix) or defined(__unix) or both.

About pshar: it will probably be removed.
g711 and rpelt would probably require some reworking to get them running without unistd.h.

@othiman othiman mentioned this pull request Sep 27, 2018
@othiman
Copy link
Contributor Author

othiman commented Sep 27, 2018

@dennisguse I found a very nice site which addresses this issue:
http://nadeausoftware.com/articles/2012/01/c_c_tip_how_use_compiler_predefined_macros_detect_operating_system#HowtodetectPOSIXandUNIX

I think we should use
#if !defined(_WIN32) && (defined(__unix__) || defined(__unix) || (defined(__APPLE__) && defined(__MACH__)))
based on the article. What do you think?

@dennisguse
Copy link
Member

@othiman You are right - this looks better.
I created an issue for this: #91

I think this is quite some manual effort.
Therefore, I believe that this could be postpone until all tests are working.
In addition, it might be problematic that this requires changing a lot of code manually.
Thus, we might end up creating some merging issues...
Anyhow, would you like to take issue #91?

PS: Could this pull request be closed?

@othiman
Copy link
Contributor Author

othiman commented Oct 2, 2018

Another thing is, what platforms should be supported in the future? E.g., it seems not very reasonable to continue supporting the VMS platform. Anyhow, this PR is outdated, so I close it.

@othiman othiman closed this Oct 2, 2018
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.

g711, rpelt: cannot be compiled on Windows
2 participants