build: Add -Werror=implicit-fallthrough compile flag#21430
build: Add -Werror=implicit-fallthrough compile flag#21430fanquake merged 2 commits intobitcoin:masterfrom
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
|
cr ACK e95d14e: patch looks correct! I've always been worried about introduction of bugs in our code due to unintentional fallthroughs. Thanks for reducing the number of things to worry about by one :) As always: explicit is better than implicit! :) |
|
Updated e95d14e -> 2f3436b (pr21430.01 -> pr21430.02, diff):
bitcoin/src/leveldb/util/hash.cc Lines 11 to 12 in c771fc0 |
|
cr re-ACK 2f3436b Consider submitting the |
|
Rebased 2f3436b -> 5bc16d2 (pr21430.02 -> pr21430.03) due to the conflicts with #21610 and #21613. |
|
cr ACK 5bc16d2: patch looks correct Thanks a lot for doing this @hebasto: the introduction of Killing bug instances is cool, but killing entire bug classes is even cooler! :) |
|
Rebased 5bc16d2 -> 45dc9f3 (pr21430.03 -> pr21430.04) due to the conflict with #22258. |
|
cr re-ACK 45dc9f3 |
|
Updated 45dc9f3 -> ebc36d9 (pr21430.04 -> pr21430.05):
|
|
Updated ebc36d9 -> 3c4c8e7 (pr21430.05 -> pr21430.06, diff): |
|
Concept ACK |
jarolrod
left a comment
jarolrod
left a comment
There was a problem hiding this comment.
ACK 3c4c8e7
This is nice to be able to detect fallthroughs going forward. Appropriate since GCC 7 is the current minimum supported GCC.
Tested that the codebase itself will throw the warning by following the same step of removing one of the added [[fallthrough]] statements as outlined here: #21430 (review)
|
Do you mind making another (final?) review? |
theStack
left a comment
theStack
left a comment
There was a problem hiding this comment.
ACK 3c4c8e7
Tested that removing an arbitrary [[fallthrough]] statment in the codebase leads to either a warning (default configuration) or an error (if configured with --enable-werror) with clang 11.0.1-2 (Debian bullseye/sid), e.g.:
hash.cpp:52:9: error: unannotated fall-through between switch labels [-Werror,-Wimplicit-fallthrough]
case 2:
^
hash.cpp:52:9: note: insert '[[fallthrough]];' to silence this warning
case 2:
^
[[fallthrough]];
hash.cpp:52:9: note: insert 'break;' to avoid fall-through
case 2:
^
break;
1 error generated.
By the way, any reason we don't --enable-werror by default? Being on the cautious side would seem to be a more sane default to me.
There might be unknown warnings in future releases of compilers that we are not aware of. Also, compilers might produce false warnings if they have bugs. So we only error on well tested and selected warnings. |
…flag 3c4c8e7 build: Add -Werror=implicit-fallthrough compile flag (Hennadii Stepanov) 014110c Use C++17 [[fallthrough]] attribute, and drop -Wno-implicit-fallthrough (Hennadii Stepanov) Pull request description: ACKs for top commit: fanquake: ACK 3c4c8e7 - looks ok to me now. Checked that warnings occur in our code & leveldb by removing a `[[fallthrough]]` or `FALLTHROUGH_INTENDED`. jarolrod: ACK 3c4c8e7 theStack: ACK 3c4c8e7 Tree-SHA512: 4dce91f0f26b8a3de09bd92bb3d7e1995e078e3a8b3ff861c4fbf6c0b32b2327d063633b07b89c4aa94a1141d7f78d46d9d43ab8df865273e342693ad30645b6
7 participants
No description provided.