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

[core] Unify unix macro #3142

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

y761823
Copy link

@y761823 y761823 commented Mar 13, 2025

In Ubuntu 20.04, gcc/clang has only lowercase unix macro, no UNIX macro.
And in channel.cpp, there are both unix and UNIX macro, unify to unix

➜  ~ echo | clang-11 -dM -E - | grep -i unix                                    
#define __unix 1
#define __unix__ 1
#define unix 1
➜  ~ echo | g++-14 -dM -E - | grep -i unix
#define __unix 1
#define __unix__ 1
#define unix 1

@maxsharabayko maxsharabayko added this to the v1.5.5 milestone Mar 17, 2025
@maxsharabayko maxsharabayko added Type: Maintenance Work required to maintain or clean up the code [build] Area: Changes in build files labels Mar 17, 2025
error message:
```
/home/runner/work/srt/srt/srtcore/channel.cpp:908:57: error: passing argument 2 to restrict-qualified parameter aliases with argument 4 [-Werror=restrict]
  908 |     const int select_ret = ::select((int)m_iSocket + 1, &set, NULL, &set, &tv);
```
@ethouris ethouris added the help wanted Indicates that a maintainer wants help on an issue or pull request label Apr 1, 2025
@ethouris
Copy link
Collaborator

ethouris commented Apr 1, 2025

There are two problems with this PR.

  1. We don't exactly know why there is the UNIX macro in use there. This actually stretches up to the times of the original codebase for SRT, UDT. The problem is, gcc has never been setting it (at least the oldest version I know) and all the information available on it is that one might at best have set it on their own. The build files (also those on UDT4) do not define it at all. So, the current state of things is that likely the UNIX macro was so far never defined in any build made by anyone for SRT. By changing it to unix you change the behavior of the existing builds and I doubt this was originally intended. For what I have seen there is that it uses exceptionally F_NDELAY flag in case when the system doesn't support the SO_RCVTIMEO socket option for UDP sockets. Changing UNIX into unix definitely isn't the right approach. There should be rather revised various versions for these conditions and see if there is any reason to have a different approach on different systems there, and how.
  2. You have modified the select function call that was specifying so far the descriptor set the same for reading and error set by removing this for the error set. Is there any merit for this change? If so, please add appropriate description and explanation for this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[build] Area: Changes in build files help wanted Indicates that a maintainer wants help on an issue or pull request Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants