Devsh-Graphics-Programming / Nabla

Vulkan, OptiX and CUDA Interoperation Modular Rendering Library and Framework for PC/Linux/Android
http://devsh.eu
Apache License 2.0
443 stars 48 forks source link

Treat all warnings as errors #685

Open AnastaZIuk opened 2 months ago

AnastaZIuk commented 2 months ago

Nabla should be compiled with /WX (for MSVC, for GCC/Clang -Werror) flag enabled for compiler and linker which treats all warnings as errors, then all of them should be eliminated and sources adjusted. Doing so may save us in future, recursive warning generation may lead to unresponsive builds taking more then half an hour, also we decrease a chance for undefined behaviours at runtime.

AnastaZIuk commented 2 months ago

this should be toggleable - introduce NBL_TREAT_WARNINGS_AS_ERRORS enabled by default and let user to override it as CMake option if needed

devshgraphicsprogramming commented 2 months ago

You'll run into problems with that, because whenever you include a 3rdparty header you get all of its warnings.

So it's not as easy as just enabling WX because you don't want to be bogged down into forking every dependency and "fixing" their warnings.

P.s. there was a story one about an intern or a junior who fixed uninitialised variables in openssl and broke it cause they were actually a source of entropy for the rand gen

AnastaZIuk commented 2 months ago

I would limit fixing them to our headers & sources only

AnastaZIuk commented 2 months ago

who fixed uninitialised variables in openssl and broke it cause they were actually a source of entropy for the rand gen

yeah I get you, though I don't think that would be a case if this happened in our codebase, I rather stick to initialize variables not to some random values which may lead to bugs, but of course it's not a case as well to fix all 3rdparty headers

AnastaZIuk commented 2 months ago

You'll run into problems with that, because whenever you include a 3rdparty header you get all of its warnings.

well another problem this could introduce an implicit vendor/toolset dependency, but since we are targeting latest toolsets and cpp standard I would not care much

AnastaZIuk commented 2 months ago

by the way, there are solutions how to disable/ignore warnings in external headers coming from 3rdparty and still treat all warnings as errors with our codebase

#pragma warning(push, 0)
// Some include(s) with unfixable warnings
#pragma warning(pop)

also https://devblogs.microsoft.com/cppblog/broken-warnings-theory/

AnastaZIuk commented 2 months ago

by the way, there are solutions how to disable/ignore warnings in external headers coming from 3rdparty and still treat all warnings as errors with our codebase

#pragma warning(push, 0)
// Some include(s) with unfixable warnings
#pragma warning(pop)

also https://devblogs.microsoft.com/cppblog/broken-warnings-theory/

with that you don't have this problem you mentioned

because whenever you include a 3rdparty header you get all of its warnings.