{cmake} About compiler warnings...

[asmaloney]

Because godot-cpp is a library used by non-godot code, its external headers should compile without warnings as much as possible. godot-cpp should adapt to the projects using it, not the other way around.

Right now a project can't turn on a lot of warnings without godot-cpp's headers producing a large number of them and it obviously prevents turning on "warnings as errors". Some of this can be worked around by wrapping the godot-cpp includes in the main project, but that's not really a great workaround.

How to choose what warnings to respect is obviously an issue, but most C++ projects seem to have at least -Wall -extra and /W3 or /W4 on in their own builds. This generally improves code quality & catches bugs.

I would like to suggest that godot-cpp should build cleanly with -Wall -extra and /W4 and then look at others that can be turned on (there are a bunch already listed in CMakeLists.txt - some of which are already included in -Wall -extra).

I'm happy to work towards this goal if it will be supported.

(Note: I haven't looked at Scons since I don't use it. No idea how this works there, but maybe it needs to happen there as well.)

[mihe]

While I'm all for enabling as many warnings as possible, just for the sake of a healthy codebase, striving to achieve zero warnings across many, potentially buggy, versions of all compilers on all platforms, for the sake of not bothering the end-user, feels like a losing, never-ending battle.

The way I typically see this solved is by the consuming project utilizing system include paths, using -isystem on GCC-like compilers and /external:I on MSVC-like compilers, which will make the compiler ignore all warnings in headers originating from there.

CMake does this for you automatically when you use a dependency through something like ExternalProject and declare the target as IMPORTED. When you use FetchContent (or simply add_subdirectory) things get trickier, as you're effectively subsuming the dependency's targets into your own CMake environment. In these cases popular libraries like to include a flag to treat its headers as SYSTEM includes (see fmt or Abseil).

Godot core does this through SCons as well in some places (see glslang and minimp3), on top of already disabling all warnings for the third-party source files.

As the comments allude to in those Godot core links, /external:I is a fairly recent addition to MSVC, starting with VS2019 16.10, and would previously require surrounding the offending includes in something like #pragma warning(push, 0), which I assume is what you mean by "wrapping the godot-cpp includes". I've relied on this myself, since it's usually fairly painless when used in conjunction with precompiled headers, which are typically force-included everywhere anyway. I don't believe disabling all warnings in this manner is a thing on GCC-like compilers though.

I guess what I'm suggesting is to follow the lead of what libraries like fmt and Abseil are doing and instead add a CMake flag to treat godot-cpp's headers as SYSTEM.

I'm not sure what the most idiomatic solution for SCons would be, but maybe something similar is needed there as well.

[asmaloney]

Thanks Mikael!

across many, potentially buggy, versions of all compilers on all platforms

For reference, in Godot 4, it looks like these are the currently supported C++ compilers:

• (win) VS2017+ (VS2019 recommended)
• (win) MinGW-w64 w/GCC 9+
• (linux/BSD) GCC 7+ or Clang 6+
• (macOS) XCode/Apple Clang- any version?
• (iOS) Xcode 11.0 (or later)

Some of those are indeed quite old.

(Aside: At this point VS2019+ should be required anyways... - see EOL list)

for the sake of not bothering the end-user

That's obviously not the only reason, as I stated ("This generally improves code quality & catches bugs."). I didn't emphasize this because I thought it was kind of obvious. In my experience, compiling with as many warnings as makes sense (I'm giving -Wconversion and -Wsign-conversion a dirty look!) and fixing them leads to more portable and, in general, cleaner code.

Compiling cleanly on as many compilers as possible is of great benefit partly because they all have slightly different warnings. If anything shows up as problematic to fix across compilers, then that's a "code smell" to me.

feels like a losing, never-ending battle

I disagree. I do agree it's a lot more difficult when a project doesn't start cleanly though 😆

-Wall -extra and /W4 really shouldn't impossible, should it?

I guess what I'm suggesting is to follow the lead of what libraries like fmt and Abseil are doing and instead add a CMake flag to treat godot-cpp's headers as SYSTEM.

If that works here, that seems like a good first step!

I still believe that it would be better for the health of this project to aspire to cleaner warning-free code.

[Faless]

I personally think warnings are very useful for the development of godot-cpp itself, as they can highlight bugs which would go undetected otherwise. As an example, both #998 and #1000 can be spotted that way.

While having zero-warnings on all possible compilers is not a reachable goal, I agree we should at least aim to support more or less what we have in upstream godot.

I had a look at enabling warning with scons + gcc (which lead to #1000) and it seems doable.

[akien-mga]

I would suggest using the same set of warnings as in upstream Godot, which we do build with -Wall -Wextra + some more and /W4, albeit with some warnings explicitly disabled because they're just overkill. A lot of the code in godot-cpp is copied or generated from Godot's core code and warnings ignored there would likely need to be ignored here too.

Here's the upstream warnings setup:

https://github.com/godotengine/godot/blob/d93b66ad4d6b1dae42cc318c16224d03bd1a1472/SConstruct#L680-L761

And indeed as pointed out, vendored thirdparty code has warnings disabled in Godot, as it's not our job to fix warnings in code we don't maintain. Similarly, users shouldn't need to worry about warnings in godot-cpp, so we should aim to fix all the ones we can, but also provide an easy way for users (both with SCons and CMake) to ignore godot-cpp warnings even if they enable -Werror//WX on their own extension code.

Here's how we force warnings off in thirdparty code for Godot:

https://github.com/godotengine/godot/blob/d93b66ad4d6b1dae42cc318c16224d03bd1a1472/methods.py#L38-L48

That indeed only works for when we compile the C/C++ code of those libraries. Sometimes warnings originate in thirdparty header files which are included by Godot code compiled with lots of warnings enabled, and as @mihe pointed out this can be worked around with some -isystem magic with GCC/Clang. It's good to know that MSVC finally added /external:I, we could start using it conditionally maybe - though thankfully it seems the warnings we did get in some thirdparty headers are not caught by MSVC currently so we could get away without it...

[asmaloney]

Small update:

I have local changes to the cmake file to use all the godot warnings.

I'm just going through and fixing as many as as I can with my setup (macOS clang) before I push a branch and start "CI-debugging" with Linux and Windows.