Port ioctl prototype check to CMake #3

Merged
MicheleC merged 2 commits from feat/fix-ioctl-prototype-check into master 4 years ago
obache commented 4 years ago
Collaborator

realted to TDE/arts#2

realted to TDE/arts#2
Owner

This looks very good – like the right solution to replace pull request #2.

This looks very good – like the right solution to replace pull request #2.
selk commented 4 years ago
Collaborator

I am going to test your PR in a moment. BTW, just one detail; unsigned long and unsigned long int are the same thing (I don't know if you want to preserve both here)

I am going to test your PR in a moment. BTW, just one detail; unsigned long and unsigned long int are the same thing (I don't know if you want to preserve both here)
obache commented 4 years ago
Poster
Collaborator

@selk "unsigned long int" v.s. "unsigned long" is the questional and WIP point.
I don't know the historical reason why aRts introduced the check.

@selk "unsigned long int" v.s. "unsigned long" is the questional and WIP point. I don't know the historical reason why aRts introduced the check.
selk commented 4 years ago
Collaborator

Far as I can see, it is safe to remove unsigned long int:

http://man7.org/linux/man-pages/man2/ioctl.2.html
https://www.freebsd.org/cgi/man.cgi?query=ioctl&sektion=2
https://man.openbsd.org/ioctl.2
https://docs.oracle.com/cd/E26505_01/html/816-5167/ioctl-2.html
Far as I can see, it is safe to remove unsigned long int: ``` http://man7.org/linux/man-pages/man2/ioctl.2.html https://www.freebsd.org/cgi/man.cgi?query=ioctl&sektion=2 https://man.openbsd.org/ioctl.2 https://docs.oracle.com/cd/E26505_01/html/816-5167/ioctl-2.html ```
selk commented 4 years ago
Collaborator

I recently tested your (WIP) patch under Musl and it worked. :-)

I recently tested your (WIP) patch under Musl and it worked. :-)
Owner

https://mirror.git.trinitydesktop.org/gitea/TDE/arts/src/branch/master/artsc/artsdsp.c#L78

long and long int are the same data type. I suggest we drop long int and update this PR and arts code accordingly so that we have only HAVE_IOCTL_INT_INT_DOTS and HAVE_IOCTL_INT_ULONG_DOTS.

Any feedback is welcome.

https://mirror.git.trinitydesktop.org/gitea/TDE/arts/src/branch/master/artsc/artsdsp.c#L78 long and long int are the same data type. I suggest we drop long int and update this PR and arts code accordingly so that we have only HAVE_IOCTL_INT_INT_DOTS and HAVE_IOCTL_INT_ULONG_DOTS. Any feedback is welcome.
obache commented 4 years ago
Poster
Collaborator

Just only HAVE_IOCTL_INT_ULONG_DOTS is sufficient, others should be the standard compliance?

Just only HAVE_IOCTL_INT_ULONG_DOTS is sufficient, others should be the standard compliance?
Owner

Tests 1 – HAVE_IOCTL_INT_INT_DOTS and 2 – HAVE_IOCTL_INT_ULONG_DOTS are necessary. Test 3 – HAVE_IOCTL_INT_ULONGINT_DOTS seems to be actually the same as 2, because long and long int determine the same thing.

Therefore, it seems that we can exclude test 3 as well as the condition in artsc/artsdsp.c.

Tests 1 – `HAVE_IOCTL_INT_INT_DOTS` and 2 – `HAVE_IOCTL_INT_ULONG_DOTS` are necessary. Test 3 – `HAVE_IOCTL_INT_ULONGINT_DOTS` seems to be actually the same as 2, because `long` and `long int` determine the same thing. Therefore, it seems that we can exclude test 3 as well as the condition in [artsc/artsdsp.c](https://mirror.git.trinitydesktop.org/gitea/TDE/arts/src/branch/master/artsc/artsdsp.c#L82).
obache changed title from WIP: Port ioctl prototype check to CMake to Port ioctl prototype check to CMake 4 years ago
obache commented 4 years ago
Poster
Collaborator

changed as suggested

changed as suggested
selk commented 4 years ago
Collaborator

It looks good. 👍

It looks good. :+1:
Owner

perfect, thanks!

perfect, thanks!
MicheleC closed this pull request 4 years ago
MicheleC deleted branch feat/fix-ioctl-prototype-check 4 years ago
MicheleC added this to the R14.0.8 release milestone 4 years ago
The pull request has been merged as 08329d9014.
Sign in to join this conversation.
No reviewers
No Milestone
No Assignees
4 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: TDE/arts#3
Loading…
There is no content yet.