Skip to content

Conversation

@clhin
Copy link
Contributor

@clhin clhin commented Nov 19, 2025

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.

@metux metux requested a review from a team November 19, 2025 08:24
@stefan11111
Copy link
Contributor

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.

@clhin
Copy link
Contributor Author

clhin commented Nov 19, 2025

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.

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.

2 participants