ocornut/imgui

Fork 0

mirror of https://github.com/ocornut/imgui.git synced 2026-11-10 19:03:30 +00:00

Clang 11 warning: Bitwise operation between different enumeration types is deprecated #3040

New issue

Closed

opened 2020-02-26 07:00:35 +00:00 by tambry · 8 comments

tambry commented

2020-02-26 07:00:35 +00:00

(Migrated from github.com)

Dear ImGui version:

Branch: 898e91f20d (master at the time of writing)

Back-end/Renderer/Compiler/OS

Back-end: custom Vulkan
Compiler: clang version 11.0.0-++20200123111717+04fd2041561-1~exp1
Operating System: Debian Unstable

My Issue/Question:
C++20's P1120R0 deprecated bitwise operations between different enums. Such code is likely to become ill-formed in C++23.
Clang 11 warns about such cases. There seems to be one case in ImGui. It should be fixed.

[33/70] Building CXX object CMakeFiles/control_station.dir/_deps/imgui-src/imgui_widgets.cpp.o
_deps/imgui-src/imgui_widgets.cpp:5576:98: warning: bitwise operation between different enumeration types ('ImGuiTreeNodeFlags_' and 'ImGuiTreeNodeFlagsPrivate_') is deprecated [-Wdeprecated-enum-enum-conversion]
    flags |= ImGuiTreeNodeFlags_CollapsingHeader | (p_open ? ImGuiTreeNodeFlags_AllowItemOverlap | ImGuiTreeNodeFlags_ClipLabelForTrailingButton : 0);
                                                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

**Dear ImGui version:** Branch: 898e91f20d5c35a9148c31596211123958464622 (master at the time of writing) **Back-end/Renderer/Compiler/OS** Back-end: custom Vulkan Compiler: clang version 11.0.0-++20200123111717+04fd2041561-1~exp1 Operating System: Debian Unstable **My Issue/Question:** C++20's [P1120R0](http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1120r0.html) deprecated bitwise operations between different `enum`s. Such code is likely to become ill-formed in C++23. [Clang 11 warns about such cases](https://reviews.llvm.org/D71576). There seems to be one case in ImGui. It should be fixed. ``` [33/70] Building CXX object CMakeFiles/control_station.dir/_deps/imgui-src/imgui_widgets.cpp.o _deps/imgui-src/imgui_widgets.cpp:5576:98: warning: bitwise operation between different enumeration types ('ImGuiTreeNodeFlags_' and 'ImGuiTreeNodeFlagsPrivate_') is deprecated [-Wdeprecated-enum-enum-conversion] flags |= ImGuiTreeNodeFlags_CollapsingHeader | (p_open ? ImGuiTreeNodeFlags_AllowItemOverlap | ImGuiTreeNodeFlags_ClipLabelForTrailingButton : 0); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1 warning generated. ```

alexfrasson commented

2020-03-02 14:51:27 +00:00

(Migrated from github.com)

It could be related to #2685

ocornut commented

2020-03-02 15:19:20 +00:00

(Migrated from github.com)

Thank you for reporting! I'm disabling the warnings for now.

C++ commitee are becoming completely non-useful, C++ is just widely broken in term of allowing actual encapsulation of implementation details and it is getting worse. If they are going to obsolete this they should allow some form of enum inheritance.

Linking to #2993, #2995, will probably adopt some form of #2995 eventually meaning modern C++ is pushing us to be less type safe.

Thank you for reporting! I'm disabling the warnings for now. C++ commitee are becoming completely non-useful, C++ is just widely broken in term of allowing actual encapsulation of implementation details and it is getting worse. If they are going to obsolete this they should allow some form of enum inheritance. Linking to #2993, #2995, will probably adopt some form of #2995 eventually meaning modern C++ is pushing us to be less type safe.

steverencontre commented

2022-04-06 22:10:53 +00:00

(Migrated from github.com)

inline constexpr int operator| (Qt::Modifier m, Qt::Key k) { return (int) m | (int) k; }

(Yes, if you want to be pedantic, use static_cast() rather than (int), but I personally prefer to use C-style casts when the subtext is, "This is something that only needs to be done to satisfy pedantry")

On balance, though, I think this is a good thing. Mixing enums willy-nilly is not sensible, any more than adding physical units is - what are metres plus kilograms FFS?! - so requiring an explicit statement of when and how they can be combined is definitely a valuable guard against errors with negligible programmer effort.

Commenting because although this is a couple of years old, it was the first hit in Google when I had the problem a few moments ago. My context was that Qt keycodes and modifiers (shift/alt/etc) are defined in different enums even though they are naturally designed to go together. Solution? One line: `inline constexpr int operator| (Qt::Modifier m, Qt::Key k) { return (int) m | (int) k; }` (Yes, if you want to be pedantic, use static_cast<int>() rather than (int), but I personally prefer to use C-style casts when the subtext is, "This is something that _only_ needs to be done to satisfy pedantry") On balance, though, I think this is a _good_ thing. Mixing enums willy-nilly is not sensible, any more than adding physical units is - what are metres plus kilograms FFS?! - so requiring an explicit statement of when and how they can be combined is definitely a valuable guard against errors with negligible programmer effort.

👍 1

mirek-fidler commented

2022-04-11 09:05:13 +00:00

(Migrated from github.com)

This is not negligible effort - your solution is like 20% of what is needed. You would need to add reversed order operator|, then probably operator& too.

Funny, got here with google search as well. On balance? Using enums for constant definition was always an established practice. If they care so much about mixing enums, why they do not limit that to e.g. named enums? Or just introduce some kind of enum that behaves like before. This is not negligible effort - your solution is like 20% of what is needed. You would need to add reversed order operator|, then probably operator& too.

steverencontre commented

2022-04-11 09:58:41 +00:00

(Migrated from github.com)

This is not negligible effort - your solution is like 20% of what is needed.

No, my solution was 100% of what I needed. It literally makes no sense to combine those two enum types in any other way, and I have no need to allow inconsistency in whether I write modifier|key or key|modifier. If you want enums that are nothing more than named ints, then use constexpr int or #define. I appreciate that it's a breaking change in the standard, but it's not one I'm unhappy about.

> This is not negligible effort - your solution is like 20% of what is needed. No, my solution was 100% of what _I_ needed. It literally makes no sense to combine those two enum types in any other way, and I have no need to allow inconsistency in whether I write modifier|key or key|modifier. If you want enums that are nothing more than named ints, then use constexpr int or #define. I appreciate that it's a breaking change in the standard, but it's not one I'm unhappy about.

👍 1

mirek-fidler commented

2022-04-11 10:53:53 +00:00

(Migrated from github.com)

constexpr int is not able to assign incremental values
I hope you are not serious about #define...

All in all, it feels like some useful feature was removed from the language for not really a good reason.

(Sorry ImGui for hijacking this debate...)

- constexpr int is not able to assign incremental values - I hope you are not serious about #define... All in all, it feels like some useful feature was removed from the language for not really a good reason. (Sorry ImGui for hijacking this debate...)

steverencontre commented

2022-04-11 11:35:06 +00:00

(Migrated from github.com)

constexpr int is not able to assign incremental values

Indeed, but why do you want to be able to do that? I would suggest that in all but a few special cases, the point of enum is to create a set of names for a particular type of thing, and mixing different types of things without any constraint is a gateway to bugs.

I grant that in older C++, enum was used as a workaround for the absence of class constants, but now we have better ways, I have no problem with workarounds becoming deprecated.

* I hope you are not serious about #define...

At least half-serious. My point being that if you don't want constraints, use a mechanism that doesn't impose any :-)

All in all, it feels like some useful feature was removed from the language

I think we'll just have to agree to differ, and it's not removing a capability wholesale, it's just forcing you to be explicit if you really want to do it. Some other languages really don't let you do that kind of thing!

> * constexpr int is not able to assign incremental values Indeed, but why do you want to be able to do that? I would suggest that in all but a few special cases, the point of enum is to create a set of names for a particular type of thing, and mixing different types of things without any constraint is a gateway to bugs. I grant that in older C++, enum was used as a workaround for the absence of class constants, but now we have better ways, I have no problem with workarounds becoming deprecated. > * I hope you are not serious about #define... At least half-serious. My point being that if you don't want constraints, use a mechanism that doesn't impose any :-) > All in all, it feels like some useful feature was removed from the language I think we'll just have to agree to differ, and it's not removing a capability wholesale, it's just forcing you to be explicit if you really want to do it. Some other languages really _don't_ let you do that kind of thing!

mirek-fidler commented

2022-04-11 14:34:08 +00:00

(Migrated from github.com)

constexpr int is not able to assign incremental values

Indeed, but why do you want to be able to do that? I would suggest that in all but a few special cases, the point of enum is to create a set of names for a particular type of thing, and mixing different types of things without any constraint is a gateway to bugs.

Basically the very same situation you have encountered with Qt. Set of keyboard constants. On some platforms, they have assigned values dictated by the platform. On others (e.g. virtual targets) it is not dictated by platform and values can be anything as long as they are different.

So my first intuitive fix for C++20 was to change everyhing to const int constants. Unfortunately broke compilation of virtual targets. And devising incremental values for like 200 contants is really boring with significant chance to introduce errors.

That said, I am thankful for your solution there, happilly adopted.. Still, I feel like C++20 forces me to fill the codebase with garbage like that (this is not only things that needed fixing).

> > * constexpr int is not able to assign incremental values > > Indeed, but why do you want to be able to do that? I would suggest that in all but a few special cases, the point of enum is to create a set of names for a particular type of thing, and mixing different types of things without any constraint is a gateway to bugs. Basically the very same situation you have encountered with Qt. Set of keyboard constants. On some platforms, they have assigned values dictated by the platform. On others (e.g. virtual targets) it is not dictated by platform and values can be anything as long as they are different. So my first intuitive fix for C++20 was to change everyhing to const int constants. Unfortunately broke compilation of virtual targets. And devising incremental values for like 200 contants is really boring with significant chance to introduce errors. That said, I am thankful for your solution there, happilly adopted.. Still, I feel like C++20 forces me to fill the codebase with garbage like that (this is not only things that needed fixing).