-
Notifications
You must be signed in to change notification settings - Fork 291
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
Don't error on warnings by default #294
Conversation
1a402c0
to
363bc02
Compare
Review status: 0 of 5 files reviewed at latest revision, 1 unresolved discussion. CMakeLists.txt, line 75 at r1 (raw file):
Perhaps move this out of the outer if block, so it doesn't depend on extra warnings being disabled. You may want Comments from Reviewable |
Reviewed 5 of 5 files at r1. Comments from Reviewable |
363bc02
to
263dc7b
Compare
Reviewed 4 of 5 files at r1, 1 of 1 files at r2. CMakeLists.txt, line 75 at r1 (raw file): Previously, iphydf wrote…> Perhaps move this out of the outer if block, so it doesn't depend on extra warnings being disabled. You may want `-Werror` with default warnings (or extra ones set through `CFLAGS`).Comments from Reviewable |
Reviewed 4 of 5 files at r1, 1 of 1 files at r2. Comments from Reviewable |
Reviewed 1 of 1 files at r2. Comments from Reviewable |
There is a really nice reasoning in the PR body. Can you put that into the comment? |
Having -Werror set by default causes users' builds to fail because toxcore is not warning-free. Failing on errors is appropriate for the development phase, e.g. when building it in a CI enviroment, but it doesn't make much sense to fail builds for users and let them figure out that they need to pass -DWARNINGS=OFF to make the library build.
263dc7b
to
f0f53db
Compare
Done. |
Having
-Werror
set by default causes users' builds to fail because toxcore is not warning-free. Failing on errors is appropriate for the development phase, when running it in CI, but it doesn't make much sense to fail builds for users and let them figure out that they need to pass-DWARNINGS=OFF
to make the library build.Also, we should really address these warnings by fixing the code that causes them.
This change is