signed flags cause clang-tidy warnings when using strict options

mkalte666 commented

2020-01-20 19:55:54 +00:00

(Migrated from github.com)

Version/Branch of Dear ImGui:

Version: v1.75 WIP
Branch: master

Back-end/Renderer/Compiler/OS

Back-ends: imgui_impl_sdl2.cpp + imgui_impl_opengl3.cpp using gl3w
Compiler:

 $ clang++ --version
clang version 9.0.1 
[...]
$ clang-tidy --version
LLVM (http://llvm.org/):
  LLVM version 9.0.1
[....]

Operating System: void linux

My Issue/Question:

The underlying type of enums in c++ is implementation specific, but it usually ends up being int [1]. HIC++ reccomends against doing bit-ops on signed types and clang-tidy will complain when one does it anyways.
Now there is reason to argue that, on an end user application, having -werr and hicpp-signed-bitwise both enabled on clang-tidy is a bit overkill, and i could just // NOLINT the few lines that will affect me, but i thought i bring it up anyway.

Standalone, minimal, complete and verifiable example: (see https://github.com/ocornut/imgui/issues/2261)

// this is enough 
ImGui::Begin("Example", nullptr, ImGuiWindowFlags_NoNav | ImGuiWindowFlags_NoResize);
ImGui::End();

becomes

someFile.cpp:12: error: use of a signed integer operand with a binary bitwise operator [hicpp-signed-bitwise,-warnings-as-errors]
    ImGui::Begin("Example", nullptr,  ImGuiWindowFlags_NoNav | ImGuiWindowFlags_NoResize);

I would suggest giving all enums an explicit type (unsigned int would be enough i think), but that would require c++11. I think some preprocessor-voodo could make sure it doesn't break when c++11 is not availble, but i do not know if something like that would be acceptable.

[1] https://timsong-cpp.github.io/cppwp/enum#dcl.enum-7

**Version/Branch of Dear ImGui:** Version: v1.75 WIP Branch: master **Back-end/Renderer/Compiler/OS** Back-ends: imgui_impl_sdl2.cpp + imgui_impl_opengl3.cpp using gl3w Compiler: ``` $ clang++ --version clang version 9.0.1 [...] $ clang-tidy --version LLVM (http://llvm.org/): LLVM version 9.0.1 [....] ``` Operating System: void linux **My Issue/Question:** The underlying type of enums in c++ is implementation specific, but it usually ends up being int [1]. HIC++ reccomends against doing bit-ops on signed types and clang-tidy will complain when one does it anyways. Now there is reason to argue that, on an end user application, having `-werr` and `hicpp-signed-bitwise` both enabled on clang-tidy is a bit overkill, and i could just // NOLINT the few lines that will affect me, but i thought i bring it up anyway. **Standalone, minimal, complete and verifiable example:** _(see https://github.com/ocornut/imgui/issues/2261)_ ``` // this is enough ImGui::Begin("Example", nullptr, ImGuiWindowFlags_NoNav | ImGuiWindowFlags_NoResize); ImGui::End(); ``` becomes ``` someFile.cpp:12: error: use of a signed integer operand with a binary bitwise operator [hicpp-signed-bitwise,-warnings-as-errors] ImGui::Begin("Example", nullptr, ImGuiWindowFlags_NoNav | ImGuiWindowFlags_NoResize); ``` I would suggest giving all enums an explicit type (unsigned int would be enough i think), but that would require c++11. I think some preprocessor-voodo could make sure it doesn't break when c++11 is not availble, but i do not know if something like that would be acceptable. [1] https://timsong-cpp.github.io/cppwp/enum#dcl.enum-7

👍 1

ocornut commented

2020-01-20 21:47:21 +00:00

(Migrated from github.com)

Hello,

Coincidentally we just ignored the equivalent MSVC warnings, also see https://github.com/ocornut/imgui/pull/2983#issuecomment-575623638

Short term we are going to ignore this as well within imgui*.cpp files, until we switch to C++11, but your code will have warnings.

Hello, Coincidentally we just ignored the equivalent MSVC warnings, also see https://github.com/ocornut/imgui/pull/2983#issuecomment-575623638 Short term we are going to ignore this as well within `imgui*.cpp` files, until we switch to C++11, but your code will have warnings.

ocornut commented

2020-01-20 21:51:32 +00:00

(Migrated from github.com)

Hmm actually I got confused, if those are triggered by clang-tidy and not by clang, if there a way to disable them?
Does something like this works?

#if __has_warning("hicpp-signed-bitwise")
#pragma clang diagnostic ignored "hicpp-signed-bitwise"
#endif

Near the top of each .cpp file?

Hmm actually I got confused, if those are triggered by clang-tidy and not by clang, if there a way to disable them? Does something like this works? ``` #if __has_warning("hicpp-signed-bitwise") #pragma clang diagnostic ignored "hicpp-signed-bitwise" #endif ``` Near the top of each .cpp file?

mkalte666 commented

2020-01-20 21:59:40 +00:00

(Migrated from github.com)

clang-tidy runs separate from the normal compilation process, though cmake allows it to be run on each build (and builds fail if you then enable clang-tidys -werr flag).

The only way to ignore clang-tidy warnings is to either not enable them in the first place, or to silence a single line https://clang.llvm.org/extra/clang-tidy/#suppressing-undesired-diagnostics.

Ignoring it for now is what i expected, but i thought pointing it out anyway might not be the worst idea. Ill just disable (or, ugh, not enable) that specific warning for now.

Thanks for the reply!

clang-tidy runs separate from the normal compilation process, though cmake allows it to be run on each build (and builds fail if you then enable clang-tidys -werr flag). The only way to ignore clang-tidy warnings is to either not enable them in the first place, or to silence a single line https://clang.llvm.org/extra/clang-tidy/#suppressing-undesired-diagnostics. Ignoring it for now is what i expected, but i thought pointing it out anyway might not be the worst idea. Ill just disable (or, ugh, not enable) that specific warning for now. Thanks for the reply!

ocornut commented

2020-01-20 22:20:34 +00:00

(Migrated from github.com)

Thanks for the clarification.

I actually have a patch to switch to unsigned int (since we use a different type for enum storage and value we can do it already). The main error I got was code in imgui_demo.cpp relying on one of the RadioButton() overload to take int*, and i switched to use the other variant already:
ocornut/imgui@e499497ec5 (diff-fb4bd618fd)

(I think that int* RadioButton overload is a little bad design, may look into obsoleting it alltogether anyway)

Test branch switching all flags to unsigned int with pre C++11:
https://github.com/ocornut/imgui/tree/features/flags_unsigned_cpp03

Had to silent some warnings in imgui_demo.cpp otherwise most compilers are passing.
Then had to fix those warnings (See in Linux>Clang):
https://github.com/ocornut/imgui/runs/399795297


../../imgui.cpp: In function ‘bool ImGui::Begin(const char*, bool*, ImGuiWindowFlags)’:
../../imgui.cpp:5825:46: warning: enumeral and non-enumeral type in conditional expression [-Wextra]
         window->DC.ItemFlags = parent_window ? parent_window->DC.ItemFlags : ImGuiItemFlags_Default_;
                                ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../imgui.cpp: In function ‘void ImGui::PopItemFlag()’:
../../imgui.cpp:6194:62: warning: enumeral and non-enumeral type in conditional expression [-Wextra]
     window->DC.ItemFlags = window->DC.ItemFlagsStack.empty() ? ImGuiItemFlags_Default_ : window->DC.ItemFlagsStack.back();
                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
g++  -m32 -I../ -I../../ -g -Wall -Wformat -Wno-zero-as-null-pointer-constant -Wno-double-promotion -Wno-variadic-macros -Wextra -pedantic -c -o imgui_demo.o ../../imgui_demo.cpp
g++  -m32 -I../ -I../../ -g -Wall -Wformat -Wno-zero-as-null-pointer-constant -Wno-double-promotion -Wno-variadic-macros -Wextra -pedantic -c -o imgui_draw.o ../../imgui_draw.cpp
../../imgui_draw.cpp: In member function ‘void ImDrawList::Clear()’:
../../imgui_draw.cpp:367:19: warning: enumeral and non-enumeral type in conditional expression [-Wextra]
     Flags = _Data ? _Data->InitialFlags : ImDrawListFlags_None;
             ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
g++  -m32 -I../ -I../../ -g -Wall -Wformat -Wno-zero-as-null-pointer-constant -Wno-double-promotion -Wno-variadic-macros -Wextra -pedantic -c -o imgui_widgets.o ../../imgui_widgets.cpp
../../imgui_widgets.cpp: In function ‘bool ImGui::ColorPicker4(const char*, float*, ImGuiColorEditFlags, const float*)’:
../../imgui_widgets.cpp:4510:74: warning: enumeral and non-enumeral type in conditional expression [-Wextra]
         flags |= ((g.ColorEditOptions & ImGuiColorEditFlags__PickerMask) ? g.ColorEditOptions : ImGuiColorEditFlags__OptionsDefault) & ImGuiColorEditFlags__PickerMask;
                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../imgui_widgets.cpp:4512:73: warning: enumeral and non-enumeral type in conditional expression [-Wextra]
         flags |= ((g.ColorEditOptions & ImGuiColorEditFlags__InputMask) ? g.ColorEditOptions : ImGuiColorEditFlags__OptionsDefault) & ImGuiColorEditFlags__InputMask;
                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

With

--window->DC.ItemFlags = parent_window ? parent_window->DC.ItemFlags : ImGuiItemFlags_Default_;
++window->DC.ItemFlags = parent_window ? parent_window->DC.ItemFlags : (ImGuiItemFlags)ImGuiItemFlags_Default_;

And it look quite silly/annoying, but I don't think it would often happen in user's codebase?

After the fix CI sees no warning:
https://github.com/ocornut/imgui/runs/399805362

I wouldn't mind committing that if I knew we have a safe later path to switch to C++11 for them, so probably going to try switching to that next.

Thanks for the clarification. I actually have a patch to switch to unsigned int (since we use a different type for enum storage and value we can do it already). The main error I got was code in imgui_demo.cpp relying on one of the `RadioButton()` overload to take `int*`, and i switched to use the other variant already: https://github.com/ocornut/imgui/commit/e499497ec547616e70c0cfdd59f3a1c681d503e2#diff-fb4bd618fdb78483fa52e52b2ff5abf4L3377 (I think that `int*` RadioButton overload is a little bad design, may look into obsoleting it alltogether anyway) Test branch switching all flags to `unsigned int` with pre C++11: https://github.com/ocornut/imgui/tree/features/flags_unsigned_cpp03 Had to silent some warnings in imgui_demo.cpp otherwise most compilers are passing. Then had to fix those warnings (See in Linux>Clang): https://github.com/ocornut/imgui/runs/399795297 ``` ../../imgui.cpp: In function ‘bool ImGui::Begin(const char*, bool*, ImGuiWindowFlags)’: ../../imgui.cpp:5825:46: warning: enumeral and non-enumeral type in conditional expression [-Wextra] window->DC.ItemFlags = parent_window ? parent_window->DC.ItemFlags : ImGuiItemFlags_Default_; ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../imgui.cpp: In function ‘void ImGui::PopItemFlag()’: ../../imgui.cpp:6194:62: warning: enumeral and non-enumeral type in conditional expression [-Wextra] window->DC.ItemFlags = window->DC.ItemFlagsStack.empty() ? ImGuiItemFlags_Default_ : window->DC.ItemFlagsStack.back(); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ g++ -m32 -I../ -I../../ -g -Wall -Wformat -Wno-zero-as-null-pointer-constant -Wno-double-promotion -Wno-variadic-macros -Wextra -pedantic -c -o imgui_demo.o ../../imgui_demo.cpp g++ -m32 -I../ -I../../ -g -Wall -Wformat -Wno-zero-as-null-pointer-constant -Wno-double-promotion -Wno-variadic-macros -Wextra -pedantic -c -o imgui_draw.o ../../imgui_draw.cpp ../../imgui_draw.cpp: In member function ‘void ImDrawList::Clear()’: ../../imgui_draw.cpp:367:19: warning: enumeral and non-enumeral type in conditional expression [-Wextra] Flags = _Data ? _Data->InitialFlags : ImDrawListFlags_None; ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ g++ -m32 -I../ -I../../ -g -Wall -Wformat -Wno-zero-as-null-pointer-constant -Wno-double-promotion -Wno-variadic-macros -Wextra -pedantic -c -o imgui_widgets.o ../../imgui_widgets.cpp ../../imgui_widgets.cpp: In function ‘bool ImGui::ColorPicker4(const char*, float*, ImGuiColorEditFlags, const float*)’: ../../imgui_widgets.cpp:4510:74: warning: enumeral and non-enumeral type in conditional expression [-Wextra] flags |= ((g.ColorEditOptions & ImGuiColorEditFlags__PickerMask) ? g.ColorEditOptions : ImGuiColorEditFlags__OptionsDefault) & ImGuiColorEditFlags__PickerMask; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ../../imgui_widgets.cpp:4512:73: warning: enumeral and non-enumeral type in conditional expression [-Wextra] flags |= ((g.ColorEditOptions & ImGuiColorEditFlags__InputMask) ? g.ColorEditOptions : ImGuiColorEditFlags__OptionsDefault) & ImGuiColorEditFlags__InputMask; ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ``` With ``` --window->DC.ItemFlags = parent_window ? parent_window->DC.ItemFlags : ImGuiItemFlags_Default_; ++window->DC.ItemFlags = parent_window ? parent_window->DC.ItemFlags : (ImGuiItemFlags)ImGuiItemFlags_Default_; ``` And it look quite silly/annoying, but I don't think it would often happen in user's codebase? After the fix CI sees no warning: https://github.com/ocornut/imgui/runs/399805362 I wouldn't mind committing that if I knew we have a safe later path to switch to C++11 for them, so probably going to try switching to that next.

ocornut commented

2020-01-20 22:48:01 +00:00

(Migrated from github.com)

Here with C++11 style
https://github.com/ocornut/imgui/tree/features/flags_unsigned_cpp11
CI:
https://github.com/ocornut/imgui/runs/399816469

Basically this:

../../imgui_demo.cpp:1793:105: warning: enumeral and non-enumeral type in conditional expression [-Wextra]
ImGuiWindowFlags window_flags = (disable_mouse_wheel ? ImGuiWindowFlags_NoScrollWithMouse : 0);

Unfortunately this seems tricky as Clang and GCC include a warning as part of -Wextra and can't seem to find how to disable it individually..
It would require using e.g. ImGuiWindowFlags_None but that means a million breakage in user code including code copied from the demo.

Here with C++11 style https://github.com/ocornut/imgui/tree/features/flags_unsigned_cpp11 CI: https://github.com/ocornut/imgui/runs/399816469 Basically this: ``` ../../imgui_demo.cpp:1793:105: warning: enumeral and non-enumeral type in conditional expression [-Wextra] ImGuiWindowFlags window_flags = (disable_mouse_wheel ? ImGuiWindowFlags_NoScrollWithMouse : 0); ``` Unfortunately this seems tricky as Clang and GCC include a warning as part of `-Wextra` and can't seem to find how to disable it individually.. It would require using e.g. `ImGuiWindowFlags_None` but that means a million breakage in user code including code copied from the demo.

mkalte666 commented

2020-01-20 23:14:02 +00:00

(Migrated from github.com)

I'm on mobile right now, but I think to get rid of that you'd need to suppress Wno-enum-compare (wich comes with Wall ?) and im not sure if that is a food idea.

I'm on mobile right now, but I think to get rid of that you'd need to suppress `Wno-enum-compare` (wich comes with `Wall` ?) and im not sure if that is a food idea.

mkalte666 commented

2020-01-21 19:36:31 +00:00

(Migrated from github.com)

FWIW, the c++11 feature branch integrates just fine in my more recent projects and does not cause any warnings (and the clang-tidy thing i came here for is gone), but i do not do anything too weird in them.

Thinking about the warning issue - are the trailing-underscore enum types used by anyone, and is them being an enum strictly required?
They could probably become constexpr unsigned int i think, as they are unscoped enums anyway.
I tried that out, and make -C examples/example_null EXTRA_WARNINGS=1 now builds without warnings.

FWIW, the c++11 feature branch integrates just fine in my more recent projects and does not cause any warnings (and the clang-tidy thing i came here for is gone), but i do not do anything too weird in them. Thinking about the warning issue - are the trailing-underscore enum types used by anyone, and is them being an enum strictly required? They could probably become `constexpr unsigned int` i think, as they are unscoped enums anyway. I tried that out, and `make -C examples/example_null EXTRA_WARNINGS=1` now builds without warnings.

ocornut commented

2020-01-21 20:15:38 +00:00

(Migrated from github.com)

The trailing underscore version mostly were created to workaround the inability to fwd declare enums pre C++11, without having to have all of them cluttering the top sections of imgui.h

You are right we could remove them. It would be useful to still have some way for IDEs to jump from the SomeFlags type right into the list of flags.

You are correct we could perhaps ditch enums as well and just use typedef, i am not acquainted enough with constexpr int in header to tell if they have any effect on what’s output from each compilation unit and passed to the linker.

The trailing underscore version mostly were created to workaround the inability to fwd declare enums pre C++11, without having to have all of them cluttering the top sections of imgui.h You are right we could remove them. It would be useful to still have some way for IDEs to jump from the SomeFlags type right into the list of flags. You are correct we could perhaps ditch enums as well and just use typedef, i am not acquainted enough with constexpr int in header to tell if they have any effect on what’s output from each compilation unit and passed to the linker.

mkalte666 commented

2020-01-21 21:00:23 +00:00

(Migrated from github.com)

Usage of constexpr values is, afaik, treated as literals as long no address is taken from the variable.
https://godbolt.org/z/Jw9wEk

However, symbols are still emitted as long as optimization is turned off.

/tmp$ cat test.cpp
constexpr int a = 1234;

void something()
{
    int b = a;
}

/tmp$  g++ -c test.cpp  -std=c++11 
/tmp$ nm test.o | c++filt 
0000000000000000 T something()
0000000000000000 r a
/tmp$ g++ -c test.cpp  -std=c++11 -O0 
/tmp$ nm test.o | c++filt 
0000000000000000 T something()
0000000000000000 r a
/tmp$ g++ -c test.cpp  -std=c++11 -O1 
/tmp$ nm test.o | c++filt 
0000000000000000 T something()

Or, in the example_null case, using constexpr:

$ nm example_null | c++filt | grep Flags 
0000000000094704 r ImGuiDragFlags_None
00000000000a22dc r ImGuiDragFlags_None
00000000000aa8fc r ImGuiDragFlags_None
0000000000094750 r ImGuiItemFlags_None
00000000000a2328 r ImGuiItemFlags_None
00000000000aa948 r ImGuiItemFlags_None
0000000000094794 r ImGuiTextFlags_None
00000000000a236c r ImGuiTextFlags_None
00000000000aa98c r ImGuiTextFlags_None
[... this goes on for quite a while ...]

With a simple -O1 though:

$ nm example_null | c++filt | grep Flags_
[ no results ] 
$ nm example_null | c++filt | grep Flags
000000000004c5fe T ImGui::CheckboxFlags(char const*, unsigned int*, unsigned int)
000000000000f5b5 T ImGui::UpdateHoveredWindowAndCaptureFlags()
$

I do not thing this would be a problem, as no one should be using them as anything else but replacement for writing literals/enums. I do not know how this effects compilation time or file size, but i suspect not that nicely, as long as .O0 is assumed.

$ stat example_null --printf="%s\n"
2025000 # with constexpr, -O0
$ stat example_null --printf="%s\n"
1964184 # without constexpr, -O0

Interestingly c++17 introduced inline constexpr variables to address this (those only ever emit one symbol and hence share an address, much like inline functions i suppose).

Usage of constexpr values is, afaik, treated as literals as long no address is taken from the variable. https://godbolt.org/z/Jw9wEk However, symbols are still emitted as long as optimization is turned off. ``` /tmp$ cat test.cpp constexpr int a = 1234; void something() { int b = a; } /tmp$ g++ -c test.cpp -std=c++11 /tmp$ nm test.o | c++filt 0000000000000000 T something() 0000000000000000 r a /tmp$ g++ -c test.cpp -std=c++11 -O0 /tmp$ nm test.o | c++filt 0000000000000000 T something() 0000000000000000 r a /tmp$ g++ -c test.cpp -std=c++11 -O1 /tmp$ nm test.o | c++filt 0000000000000000 T something() ``` Or, in the example_null case, using constexpr: ``` $ nm example_null | c++filt | grep Flags 0000000000094704 r ImGuiDragFlags_None 00000000000a22dc r ImGuiDragFlags_None 00000000000aa8fc r ImGuiDragFlags_None 0000000000094750 r ImGuiItemFlags_None 00000000000a2328 r ImGuiItemFlags_None 00000000000aa948 r ImGuiItemFlags_None 0000000000094794 r ImGuiTextFlags_None 00000000000a236c r ImGuiTextFlags_None 00000000000aa98c r ImGuiTextFlags_None [... this goes on for quite a while ...] ``` With a simple -O1 though: ``` $ nm example_null | c++filt | grep Flags_ [ no results ] $ nm example_null | c++filt | grep Flags 000000000004c5fe T ImGui::CheckboxFlags(char const*, unsigned int*, unsigned int) 000000000000f5b5 T ImGui::UpdateHoveredWindowAndCaptureFlags() $ ``` I do not thing this would be a problem, as no one should be using them as anything else but replacement for writing literals/enums. I do not know how this effects compilation time or file size, but i suspect not that nicely, as long as .O0 is assumed. ``` $ stat example_null --printf="%s\n" 2025000 # with constexpr, -O0 $ stat example_null --printf="%s\n" 1964184 # without constexpr, -O0 ``` Interestingly c++17 introduced inline constexpr variables to address this (those only ever emit one symbol and hence share an address, much like inline functions i suppose).

Rows
Columns

signed flags cause clang-tidy warnings when using strict options #2993