Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Treat all warnings as errors #685

Open
AnastaZIuk opened this issue Apr 27, 2024 · 7 comments
Open

Treat all warnings as errors #685

AnastaZIuk opened this issue Apr 27, 2024 · 7 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@AnastaZIuk
Copy link
Member

AnastaZIuk commented Apr 27, 2024

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 AnastaZIuk added enhancement New feature or request help wanted Extra attention is needed good first issue Good for newcomers labels Apr 27, 2024
@AnastaZIuk
Copy link
Member Author

AnastaZIuk commented Apr 27, 2024

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
Copy link
Member

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
Copy link
Member Author

I would limit fixing them to our headers & sources only

@AnastaZIuk
Copy link
Member Author

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
Copy link
Member Author

AnastaZIuk commented Apr 27, 2024

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
Copy link
Member Author

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
Copy link
Member Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants