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

Mark alternative ImColor constructors as constexpr #6656

Merged
merged 1 commit into from
Jul 29, 2023

Conversation

EggsyCRO
Copy link
Contributor

An ImColor variable cannot be marked as constexpr / constinit when constructed using one of these constructors, as they are not marked as constexpr.

An ImColor variable cannot be marked as constexpr / constinit when constructed using one of these constructors, as they are not marked as constexpr.
@ocornut ocornut merged commit 7c5b0e8 into ocornut:master Jul 29, 2023
@ocornut
Copy link
Owner

ocornut commented Jul 29, 2023

Merged, thank you!

@ocornut
Copy link
Owner

ocornut commented Jul 29, 2023

This is actually breaking build with VS2015

1>c:\omar\work\imgui\imgui.h(2471): error C3250: 'sc': declaration is not allowed in 'constexpr' function body
1>c:\omar\work\imgui\imgui.h(2471): error C3249: illegal statement or sub-expression for 'constexpr' function

But works with VS2017 onward.

@ocornut
Copy link
Owner

ocornut commented Jul 29, 2023

Also see MinGW

https://github.com/ocornut/imgui/actions/runs/5701044022/job/15451390799

In file included from main.cpp:4:
../../imgui.h: In constructor 'ImColor::ImColor(int, int, int, int)':
../../imgui.h:2471:197: error: 'constexpr' constructor does not have empty body
     constexpr ImColor(int r, int g, int b, int a = 255)             { float sc = 1.0f / 255.0f; Value.x = (float)r * sc; Value.y = (float)g * sc; Value.z = (float)b * sc; Value.w = (float)a * sc; }

and GCC
https://github.com/ocornut/imgui/actions/runs/5701044022/job/15451390966

In file included from main.cpp:4:
../../imgui.h: In constructor ‘ImColor::ImColor(int, int, int, int)’:
../../imgui.h:2471:197: error: ‘constexpr’ constructor does not have empty body
 2471 |     constexpr ImColor(int r, int g, int b, int a = 255)             { float sc = 1.0f / 255.0f; Value.x = (float)r * sc; Value.y = (float)g * sc; Value.z = (float)b * sc; Value.w = (float)a * sc; }

I don't understand why CI didn't run on this prior to merging, this would be something else to investigate.

ocornut added a commit that referenced this pull request Jul 29, 2023
@EggsyCRO
Copy link
Contributor Author

Not sure why it would break the build on these compilers. I only tested on VS2022 with v143 build tools. I assume this can be fixed by declaring the sc variable as constexpr or removing it altogether (by multiplying each of the values by 1 / 255).

@EggsyCRO
Copy link
Contributor Author

After testing it on godbolt.org, I was unable to reproduce the issue on any C++11 compiler, even as far back as x86-64 gcc 6.1

https://godbolt.org/z/e3W4jPWTE

@ocornut
Copy link
Owner

ocornut commented Jul 29, 2023

I was unable to reproduce the issue on any C++11 compiler, even as far back as x86-64 gcc 6.1

I am not sure either why it's happening you can see CI actions above are failing.

I assume this can be fixed by declaring the sc variable as constexpr or removing it altogether (by multiplying each of the values by 1 / 255).

I'll do that. I verified that 1/255 is not evaluated 4 times in VS2015 debug mode (constants are folded) and that's good enough for me
image

ocornut added a commit that referenced this pull request Jul 29, 2023
…6656)

Earlier 7c5b0e8 broke with VS2015 and some other MingGW/GCC setups.
@ocornut
Copy link
Owner

ocornut commented Jul 29, 2023

I have pushed 19ae142

Note that I tried to mark the variable as constexpr and it didn't work in VS2015.
Also the GCC/MinGW warning being "constructor does not have empty body" I'm assuming it wouldn't work there.

@PathogenDavid
Copy link
Contributor

I don't understand why CI didn't run on this prior to merging, this would be something else to investigate.

Do you remember if you approved the workflow run? CI doesn't run by default on PRs from first time contributors. (See here for details.)

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.

3 participants