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

[3.x] Fix #if *_ENABLED inconsistencies, should check if defined #87272

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

halotroop2288
Copy link
Contributor

@halotroop2288 halotroop2288 commented Jan 16, 2024

As discussed on RocketChat, these checks are probably written incorrectly.

@halotroop2288 halotroop2288 changed the title Fix if *_ENABLED bugs part 1 Fix if *_ENABLED bugs Jan 16, 2024
@halotroop2288 halotroop2288 changed the title Fix if *_ENABLED bugs Fix #if *_ENABLED bugs Jan 16, 2024
@AThousandShips AThousandShips added this to the 3.6 milestone Jan 16, 2024
@AThousandShips AThousandShips changed the title Fix #if *_ENABLED bugs [3.x] Fix #if *_ENABLED bugs Jan 16, 2024
@halotroop2288 halotroop2288 marked this pull request as ready for review January 16, 2024 20:20
@halotroop2288 halotroop2288 requested a review from a team as a code owner January 16, 2024 20:20
@AThousandShips
Copy link
Member

Please squash your commits into one, see here

@halotroop2288
Copy link
Contributor Author

Please squash your commits into one

There's a button for that. You don't have to make web contributors do all the work.

Squash and merge

@AThousandShips
Copy link
Member

AThousandShips commented Jan 16, 2024

We don't do that for various reasons, it's part of the workflow 🙂, see that link, we don't squash commits because it:

  • Doesn't allow it and merge commits, and we want merge commits, because they make reverting or updating things easier, as well as cherry picking
  • It resets the author verification and muddles the history
  • There are times when multiple commits are accepted, but it depends on the PR, so having a universal squash rule wouldn't work, it would also mean more work again for the production team member merging it to remember and decide on that, as opposed to being done already before merge by having the contributor do it

We're well aware of this feature and there's very good reasons we don't use it, and it's not much work, and it saves maintainers work who have to merge each PR

@halotroop2288
Copy link
Contributor Author

It's squashed. Do you need anything else?

@AThousandShips
Copy link
Member

No it looks good otherwise 🙂

@lawnjelly
Copy link
Member

lawnjelly commented Jan 17, 2024

I didn't write this, but a quick test in godbolt suggests in the existing format:

If e.g. IPHONE_ENABLED is defined as 1 or true, then the code underneath is compiled.
If it is false, then the code underneath does not compile.

If it is undefined, the compiler will flag an error:

error: #elif with no expression

So by adding the defined the PR may have changed the logic (and maybe introduced bugs?), it will compile the code underneath whether or not the ENABLED is defined as true or false.

So this may need a little double checking if to be changed. The existing code is valid c++, but just open to mis-interpretation (and could indeed be a copy pasta error). Afaik the ENABLED seem to be set in the scons scripts, so it should not be set, but I wonder why it is compiling at all if e.g. IPHONE_ENABLED is undefined. 🤔

I have limited time to check this, but just wanted to flag this and make sure it had been tested.

Pinging @clayjohn as at least some of this is his code (although it may be copy pasted for the #elif).

@akien-mga
Copy link
Member

akien-mga commented Jan 17, 2024

There's a difference between passing a define on the command line with -DSOME_DEFINE and defining it without value with #define SOME_DEFINE. The former defines it as 1, and is equivalent to -DSOME_DEFINE=1 or #define SOME_DEFINE 1. The latter defines it as empty and #if SOME_DEFINE will fail with error: #if with no expression.

Notably, this also applies to names which have not been defined on the command line. If SOME_DEFINE doesn't exist, #if SOME_DEFINE actually compiles and is equivalent to #if 0 (but SOME_DEFINE itself isn't 0, not sure how the preprocessor sees it exactly).

That's why this code "works" e.g. on Android where only ANDROID_ENABLED is defined, without throwing a compile-time error on #elif IPHONE_ENABLED which comes before checking #elif ANDROID_ENABLED. If we defined #define IPHONE_ENABLED (without value), then it would fail.

Test script to confirm that:

// Compile with: g++ main.cpp -DD -DE=1 -DF=0

#include <cstdio>

#define A
#define B 1
#define C 0
// D passed on command line with -DD
// E passed on command line with -DE=1
// F passed on command line with -DF=0
// no G

#define _STR(m_x) #m_x
#define _MKSTR(m_x) _STR(m_x)

int main() {
	printf("A: %s\n", _MKSTR(A));
	printf("B: %s\n", _MKSTR(B));
	printf("C: %s\n", _MKSTR(C));
	printf("D: %s\n", _MKSTR(D));
	printf("E: %s\n", _MKSTR(E));
	printf("F: %s\n", _MKSTR(F));
	printf("G: %s\n", _MKSTR(G));

/*
// Fails with: error: #if with no expression
#if A
	printf("A is true.\n");
#endif
*/
#if B
	printf("B is true.\n");
#endif
#if C
	printf("C is true.\n");
#endif
#if D
	printf("D is true.\n");
#endif
#if E
	printf("E is true.\n");
#endif
#if F
	printf("F is true.\n");
#endif
#if G
	printf("G is true.\n");
#endif
	return 0;
}

Prints:

A: 
B: 1
C: 0
D: 1
E: 1
F: 0
G: G
B is true.
D is true.
E is true.

All ..._ENABLED defines are passed on the command line and thus pass both #if ..._ENABLED and #if defined(..._ENABLED) checks. Still, we shouldn't count on this and the de facto convention is that we consider them true if they are defined, precisely to avoid having these discussions over and over again :P

Numbers in the codebase speak for themselves (master branch):

$ rg -g'!thirdparty' "#if defined.*ENABLED" | wc -l
261
$ rg -g'!thirdparty' "#ifdef .*ENABLED" | wc -l
2432
$ rg -g'!thirdparty' "#elif defined.*ENABLED" | wc -l
14
$ rg -g'!thirdparty' "#if [^d\!].*ENABLED" | wc -l
8
$ rg -g'!thirdparty' "#elif [^d\!].*ENABLED" | wc -l
4

For that last one it's 7 occurrences in the 3.x branch, which are the ones addressed by this PR. To me the code as is is clearly wrong, so this PR goes in the right direction.

@akien-mga akien-mga changed the title [3.x] Fix #if *_ENABLED bugs [3.x] Fix #if *_ENABLED inconsistencies, should check if defined Jan 17, 2024
@akien-mga
Copy link
Member

This PR should likely also assess the cases where #if [^d\!].*ENABLED and not just #elif, which have the same problem. That would fully solve the inconsistency.

And this all needs to be done for the master branch in priority over 3.x. Code style fixes are mainly relevant for the branch which we aim to maintain for the next decade, which is the master branch. There are lots of style inconsistencies in the 3.x branch which we opted not to resolve because it's a lot of busywork for a branch which is predominantly in maintenance support.

I'll make a master PR.

@akien-mga
Copy link
Member

This PR should likely also assess the cases where #if [^d\!].*ENABLED and not just #elif, which have the same problem. That would fully solve the inconsistency.

I amended the commit with the #if inconsistencies resolved too.

I'll make a master PR.

Done: #87286.

Verified

This commit was signed with the committer’s verified signature.
akien-mga Rémi Verschelde
Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Should be good to go.

Also included the minor style fix from #87271 in this PR as it's also just fixing a pre-processor macro inconsistency.

@akien-mga akien-mga merged commit dfb03a3 into godotengine:3.x Jan 17, 2024
13 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@halotroop2288 halotroop2288 deleted the patch-3 branch January 17, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants