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

Defines not possible to evaluate #44

Open
Ldash4 opened this issue Sep 20, 2023 · 2 comments
Open

Defines not possible to evaluate #44

Ldash4 opened this issue Sep 20, 2023 · 2 comments

Comments

@Ldash4
Copy link

Ldash4 commented Sep 20, 2023

Due to how the "conditionals" field is constructed, there are ambiguous situations that arise when evaluating them.
Given this series of defines, we can see that we expect both to pass. However only the first will be, if we update our list of defines after the condition evaluates to true.

#ifndef MAYBE_DEFINED_EXTERNALLY
#define MAYBE_DEFINED_EXTERNALLY A_DEFAULT // (will pass) conditionals: [{ "condition": "ifndef", "expression": "MAYBE_DEFINED_EXTERNALLY" }]
#define RELATED_THING A_DEFAULT_AS_WELL // (will not pass) conditionals: [{ "condition": "ifndef", "expression": "MAYBE_DEFINED_EXTERNALLY" }]
#endif // MAYBE_DEFINED_EXTERNALLY

You can see how that series of defines would result in the same data as these, even though they have different behavior.

#ifndef MAYBE_DEFINED_EXTERNALLY
#define MAYBE_DEFINED_EXTERNALLY A_DEFAULT
#endif // MAYBE_DEFINED_EXTERNALLY
#ifndef MAYBE_DEFINED_EXTERNALLY
#define RELATED_THING A_DEFAULT_AS_WELL
#endif // MAYBE_DEFINED_EXTERNALLY

This happens in practice for the IM_COL32_(R|G|B|A)_SHIFT defines:

#ifndef IM_COL32_R_SHIFT
#ifdef IMGUI_USE_BGRA_PACKED_COLOR
#define IM_COL32_R_SHIFT    16
#define IM_COL32_G_SHIFT    8
#define IM_COL32_B_SHIFT    0
#define IM_COL32_A_SHIFT    24
#define IM_COL32_A_MASK     0xFF000000
#else
#define IM_COL32_R_SHIFT    0
#define IM_COL32_G_SHIFT    8
#define IM_COL32_B_SHIFT    16
#define IM_COL32_A_SHIFT    24
#define IM_COL32_A_MASK     0xFF000000
#endif // #ifdef IMGUI_USE_BGRA_PACKED_COLOR
#endif // #ifndef IM_COL32_R_SHIFT
@Ldash4
Copy link
Author

Ldash4 commented Sep 21, 2023

Thinking this through, it's a bit tricky to represent. Maybe something like this could work?

# defines
[
	{
		"conditional": { "condition": "ifndef", "expression": "IM_COL32_R_SHIFT" },
		"true_case": {
			{
				"conditional": { "condition": "ifdef", "expression": "IMGUI_USE_BGRA_PACKED_COLOR" },
				"true_case": {
					"defines": [
						{ "name": "IM_COL32_R_SHIFT", "content": "16" },
						{ "name": "IM_COL32_G_SHIFT", "content": "8" },
						{ "name": "IM_COL32_B_SHIFT", "content": "0" },
						{ "name": "IM_COL32_A_SHIFT", "content": "24" },
						{ "name": "IM_COL32_A_MASK", "content": "0xFF000000" },
					],
				},
				"false_case": {
					"defines": [
						{ "name": "IM_COL32_R_SHIFT", "content": "0" },
						{ "name": "IM_COL32_G_SHIFT", "content": "8" },
						{ "name": "IM_COL32_B_SHIFT", "content": "16" },
						{ "name": "IM_COL32_A_SHIFT", "content": "24" },
						{ "name": "IM_COL32_A_MASK", "content": "0xFF000000" },
					],
				},
			}
		},
	},
]

This would be a special case only for defines, since this is the only section where it gets weird.

  • "defines" is an optional array of defines at the current define scope.
  • "condition" is the condition as before. There can only logically be one condition at a time though, to avoid this problem.
  • "true_case" is required if "condition" has a value
  • "false_case" is optional if "condition" has a value

@ShironekoBen
Copy link
Collaborator

I have to admit I'd not really considered the case of evaluating conditional prerequisites on defines themselves, only on other declarations (my assumption being that other languages likely didn't support defines, or wouldn't want to import the C ones as they are mostly fairly C-specific...

Internally the DOM representation of ifdefs is closer to the representation you propose here, so it's definitely not impossible to implement, but I'm slightly nervous about trying to expose that in the JSON because it feels like it will make everything much more complex - consistency would suggest that other declarations should be handled in a similar way, but then we end up needing to wrap everything in conditional blocks... (and also start caring about declaration ordering in the JSON, too)

From what I can see the only case where this is actually an issue right now is the IM_COL32... macros - given their nature I'm wondering if it would make more sense to just add a special-case for those for now that optionally removes the guard define (making the assumption that it's actually pretty rare for people to override them, at least in a scenario where bindings are involved). Does that sound like it would be a sensible solution in the immediate term?

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

No branches or pull requests

2 participants