Fixing Many Compile Warnings/Errors With Clang

[geometrian]

Version/Branch of Dear ImGui:

Version: 72096bf6
Branch: master

Back-end/Renderer/Compiler/OS

Back-ends: imgui_impl_opengl2.cpp + imgui_impl_opengl3.cpp + imgui_impl_sdl.cpp + imgui_impl_sdlrenderer.cpp
Compiler: Clang 14.0.5
Operating System: Win 10 Pro 21H2

My Issue/Question:

There are various compile warnings / errors with Clang compiled with certain warnings. Some effort has clearly been made in the past to fix or ignore them, but it is now out-of-date, and was never complete. Fortunately, fixing each one is simple, even if there are quite a few. I'll list the ones I ran into here for your convenience:

• imgui_impl_opengl2.cpp:59 includes <GL/gl.h>. Should be <gl/GL.h> (at least on my system; because everything is terrible, I know that this varies), or ignore -Wnonportable-system-include-path.
• imgui_impl_opengl2.cpp:(many) using NULL instead of nullptr. In C++, using NULL is always wrong; nullptr is a typesafer drop-in replacement. This should be changed globally. As a temporary workaround, ignore -Wzero-as-null-pointer-constant.
• imgui_impl_opengl2.cpp:213 implicit float conversion of clip_max. Could improve or ignore -Wimplicit-int-float-conversion.
• imgui_impl_opengl2.cpp:50,53 doesn't understand how the macros are used in the OpenGL header. Add ignore for Wunused-macros.
• imgui_impl_opengl3.cpp:89 add ignore for Wunused-macros.
• imgui_impl_opengl3.h:24 using NULL instead of nullptr. Note that -Wzero-as-null-pointer-constant was disabled in the ".cpp" file, but not in the header it includes. The right solution is instead using nullptr, of course.
• imgui_impl_opengl3_loader.h:97,601 The correct file is <Windows.h>. Lowercase has always been incorrect.
• imgui_impl_opengl3_loader.h:612 broken function pointer cast. Internet doesn't know how to fix. Ignoring -Wcast-function-type would shut it up, but maybe there's a 'right' way.
• imgui_impl_sdl.cpp:(many) using NULL instead of nullptr.
• imgui_impl_sdl.cpp:364,501–524 extraneous semicolons; debatably an issue in SDL.
• imgui_impl_sdl.cpp:547,552 implicit float conversion; improve or ignore -Wimplicit-int-float-conversion.
• imgui_impl_sdlrenderer.cpp:(many) using NULL instead of nullptr.
• imgui_impl_sdlrenderer.cpp:169,170 implicit float conversion; improve or ignore -Wimplicit-int-float-conversion.
• imgui_impl_sdlrenderer.cpp:177,178 ignore -Wcast-align.
• imgui_impl_sdlrenderer.cpp:191,192 implicit sign conversion; improve or ignore -Wsign-conversion.

[ocornut]

Thanks for reporting!

Fortunately, fixing each one is simple, even if there are quite a few.

There are often reasons making things not "simple". It would have be easier to have a list of actual warning output, or even a PR, because it is unlikely there are all going to be fixed immediately and the line number are already hard to track today.

The general policy IHMO is you should not compile third-party code with stringent warnings. You trust the code or you don't, the warnings are meaningless. A few things like warnings on "unused macros" on including a header file are pretty much counter-productive, if not nefarious warnings. It's not our job to support that kind of things and I have issue with conforming or ignoring to some of them (unused-macros in particularly), bowing down to their sheer stupidity enables them to grow while really they shouldn't be used.

imgui_impl_opengl2.cpp:59 includes <GL/gl.h>. Should be <gl/GL.h>

I am seeing both:

imgui_impl_opengl3_loader.h:97,601 The correct file is <Windows.h>. Lowercase has always been incorrect.

It is windows.h on MinGW and that one is the one usually complaining about casing.
We could add an ifdef in our generator gl3w_stripped/template/gl3w.h however the second one is dynamically pulled from https://www.khronos.org/registry/OpenGL/api/GL/glcorearb.h which I think highlight some of the futility of that fight.

imgui_impl_sdl.cpp:547,552 implicit float conversion; improve or ignore -Wimplicit-int-float-conversion.

I don't know where they are and the line number don't seem to match.

I took the liberty to add [] markers in your list to make it checkable and check the things that I believe are fixed now (7 out of 15 fixed by c54230d).

[ocornut]

Apart from those two

imgui_impl_sdl.cpp:547,552 implicit float conversion; improve or ignore -Wimplicit-int-float-conversion.

and

imgui_impl_sdlrenderer.cpp:191,192 implicit sign conversion; improve or ignore -Wsign-conversion.

I don't think it is worth fixing the other.
Since I can't repro your warnings presently details would be good.

[geometrian]

Hi. Thanks for fixing some of the warnings so far! Addressing issues raised below, in-order:

---

The general policy IHMO is you should not compile third-party code with stringent warnings. You trust the code or you don't, the warnings are meaningless.

For 3rd-party code I sortof agree: warnings can indicate a problem upstream, and should be reported to the maintainers. If this doesn't undermine your trust in the package, you may continue to use it. I.e., if the code works, it's not your problem.

However, note that ImGui straddles a line between 3rd-party and 1st-party, in that it is written by a 3rd-party, but it is also explicitly encouraged to be #included directly into the client code as 1st-party source:

[...] self-contained within a few platform-agnostic files which you can easily compile in your application/engine. [...]

No specific build process is required. You can add the .cpp files to your existing project.

(Emphasis yours.) This gives rise to:

[...]warnings on "unused macros" on including a header file are pretty much counter-productive, if not nefarious warnings. It's not our job to support that kind of thing[...]

I agree that some warnings are spurious or even antipattern-encouraging and, while one could debate which ones are which, I'm going after a simple, common case here: boss says -Wall -Werror and demands the code to compile. Surely, one does not have to accede to 'fixing' the 'problems' raised by -Wall . . . but it should at least work, you know? Silence such warnings.

---

Regarding gl.h/GL.h and Windows.h/windows.h, I agree with the conclusion that other conventions are used with considerable frequency outside the norm. (I dunno about GL, but I'd consider the 'default' for Windows to be Windows.h.)

Thus, I retract my statement that the casing should be changed as a matter of correctness per-se, but I do think the warning does need to be handled somehow. Perhaps the warning should be ignored for Clang (Is it true that every platform that includes Windows.h is case-insensitive? Probably yes?), or there should be some attempt to select the correctly cased header depending on which platform it is.

---

imgui_impl_sdl.cpp:547,552 implicit float conversion; improve or ignore -Wimplicit-int-float-conversion.

I can't easily get the exact compile I used, but I think this is complaining about the lines:

io.DisplayFramebufferScale = ImVec2((float)display_w / w, (float)display_h / h);
//[...]
io.DeltaTime = bd->Time > 0 ? (float)((double)(current_time - bd->Time) / frequency) : (float)(1.0f / 60.0f);

E.g. in the first w and h are not explicitly cast to float, so you get float dived by int and an implicit cast. The warning is spurious in this case, but I note that for me personally it has proven useful time and again in finding issues.

imgui_impl_sdlrenderer.cpp:191,192 implicit sign conversion; improve or ignore -Wsign-conversion.

SDL_RenderGeometryRaw(bd->SDLRenderer, tex,
    xy, (int)sizeof(ImDrawVert),
    color, (int)sizeof(ImDrawVert),
    uv, (int)sizeof(ImDrawVert),
    cmd_list->VtxBuffer.Size - pcmd->VtxOffset,
    idx_buffer + pcmd->IdxOffset, pcmd->ElemCount, sizeof(ImDrawIdx));

This doesn't like e.g. adding unsigned pcmd->VtxOffset to signed cmd_list->VtxBuffer.Size. The warning is calling attention to the fact that the size is (likely suboptimally) signed.

[ocornut]

(Is it true that every platform that includes Windows.h is case-insensitive? Probably yes?

That's not the case for MinGW.

I blindly fixed the remaining warning with 8a6911b.