xserver icon indicating copy to clipboard operation
xserver copied to clipboard

events.c: reverse macro order

Open clhin opened this issue 1 month ago • 3 comments

This reversal ensures the macro is well defined at the expense of a few extra LOC. The preprocessor runs sequentially, so XINERAMA is evaluated and the correct version of the code is substituted, the the DIX macro is evaluated, as opposed to previously where the DIX macro was evaluated but the XINERAMA macro was inside of it, leaving how and when it is defined, undefined.

clhin avatar Nov 19 '25 02:11 clhin

I don't see what this achieves other that making the code harder to read.

While technically UB, we aren't doing hacky stuff with directives in macro arguments, so it should be fine on all compilers we care about.

All these identifiers are reserved in C99, and using them is UB: https://en.cppreference.com/w/c/language/identifier.html

This includes names starting with 2 underscores, with an underscore and a capital letter, or ending with _t.

A standards-compliant compiler is free to replace the entire code with rm -rf /* if any of these reserved identifiers are used.

Such a compiler would be unusable.

It is pointless to worry about such things in X server code. We build with -fno-strict-aliasing, we don't need to care about strict standards complience or worry about potential compilers engaging is language lawyering.

If @metux wants this, I defer to him, but I really don't see how this change helps.

stefan11111 avatar Nov 19 '25 08:11 stefan11111

I'd have to respectfully disagree. While we do repeat ourself a little bit here, the current arrangement, in my opinion is confusing. Not only do you have to understand the if statement, but that if statement will be cut off if you do not set XINERAMA, meaning that if statement you just looked at now no longer exists, and you leave line 6011 with a misplaced hanging indent. The code I propose now clearly shows the user what the code looks like with and without xinerama, without relying on undefined behavior.

clhin avatar Nov 19 '25 12:11 clhin

Furthermore, after recent events with Cloudflare, we see that protecting against undefined behavior is not just about past compilers but future ones. There will inevitably come a day where some overzealous rust developer makes a rust c compiler, distributions adopt it, and it simply calls .unwind() when it has to handle UB, and somehow X11 will be blamed. So if there are no further conceptual issues I would like this merged.

clhin avatar Nov 22 '25 01:11 clhin