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

Make xbanish C99 compliant #70

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

Make xbanish C99 compliant #70

wants to merge 3 commits into from

Conversation

iacore
Copy link

@iacore iacore commented Apr 12, 2022

No description provided.

xbanish.c Outdated
#include <X11/Xlib.h>
#include <X11/Intrinsic.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I remember getting a failed compilation report due to this. It includes the header but doesn't actually link against the library.
https://bugs.gentoo.org/806258#c3

xbanish.c Outdated Show resolved Hide resolved
@@ -15,16 +15,17 @@
* OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
*/

#define _POSIX_C_SOURCE 2
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO it's better to define feature test macros in the Makefile under CPPFLAGS variable. But since it's a single file source, probably doesn't matter much.

jcs added a commit that referenced this pull request Apr 13, 2022
@N-R-K
Copy link
Contributor

N-R-K commented Apr 13, 2022

@jcs Regarding a7ebe36 : <signal.h> is also unused, was included in 725c614. I assume that was because @Ckath's original implementation was using alarm instead of the XSync* functions.

@Ckath
Copy link
Contributor

Ckath commented Apr 13, 2022

damn, and I remember going over the diff multiple times making sure I didnt accidentally pull in more junk after writing out the original alarm related signal mess. shame. must've steered too much on suggested changes

@iacore
Copy link
Author

iacore commented Apr 18, 2022

Should be fine to merge now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants