refactor: Fix unreachable code in init arg checks #19131

pull jonathanschoeller wants to merge 2 commits into bitcoin:master from jonathanschoeller:fix-Wunreachable-code-loop-increment changing 4 files +21 −4

jonathanschoeller commented at 8:25 AM on June 1, 2020: none
Closes: #19017

In #19015 it's been suggested that we add some new compiler warnings to our build. Some of these, such as -Wunreachable-code-loop-increment, generate warnings. We'll likely want to fix these up if we're going to turn these warnings on.
```
init.cpp:969:5: warning: loop will run at most once (loop increment never executed) [-Wunreachable-code-loop-increment]
     for (const auto& arg : gArgs.GetUnsuitableSectionOnlyArgs()) {
     ^~~
 1 warning generated.
```
https://github.com/bitcoin/bitcoin/blob/aa8d76806c74a51ec66e5004394fe9ea8ff0fac4/src/init.cpp#L968-L972

To fix this, collect all errors, and output them in a single error message after the loop completes. This resolves the unreachable code warning, and avoids popup hell that could result from outputting a seperate message for each error or warning one by one.
fanquake added the label Refactoring on Jun 1, 2020
practicalswift commented at 9:08 AM on June 1, 2020: contributor

Concept ACK: thanks for fixing this and thereby allowing us to catch more severe issues using -Wunreachable-code-loop-increment going forward!

Great first time contribution and great rationale section in the PR description.

Warm welcome as a contributor @jonathanschoeller! :)
in src/init.cpp:971 in 34c4fc94ad outdated
```
 963 | @@ -964,17 +964,29 @@ bool AppInitParameterInteraction()
 964 |  
 965 |      // also see: InitParameterInteraction()
 966 |  
 967 | -    // Warn if network-specific options (-addnode, -connect, etc) are
 968 | +    // Error if network-specific options (-addnode, -connect, etc) are
 969 |      // specified in default section of config file, but not overridden
 970 |      // on the command line or in this network's section of the config file.
 971 |      std::string network = gArgs.GetChainName();
 972 | -    for (const auto& arg : gArgs.GetUnsuitableSectionOnlyArgs()) {
```
MarcoFalke commented at 10:33 AM on June 1, 2020:

I don't think this line needs to change. Below you can simply check for errors.empty(), which should do the exactly same.

jonathanschoeller commented at 10:40 AM on June 1, 2020:

class "bilingual_str" has no member "empty".

Would it be reasonable to use errors.translated.empty() for this purpose?

hebasto commented at 10:43 AM on June 1, 2020:

class "bilingual_str" has no member "empty".

Would it be reasonable to use errors.translated.empty() for this purpose?

Feel free to get it from https://github.com/bitcoin/bitcoin/pull/18927/commits/1df513531f04ef31d55d2fa2acc146d526745788 :)

MarcoFalke commented at 10:46 AM on June 1, 2020:

Oh, is that not yet merged. Sorry didn't realize

in src/init.cpp:976 in 34c4fc94ad outdated

 980 | +    if (!unsuitable_args.empty()) {
 981 | +        return InitError(errors);
 982 |      }
 983 |  
 984 |      // Warn if unrecognized section name are present in the config file.
 985 | -    for (const auto& section : gArgs.GetUnrecognizedSections()) {

MarcoFalke commented at 10:33 AM on June 1, 2020:

Same

MarcoFalke approved
MarcoFalke commented at 10:34 AM on June 1, 2020: member

Nice. ACK with or without the nit addressed
MarcoFalke commented at 10:35 AM on June 1, 2020: member

Oh, it looks like you might have to adjust the tests for the longer error messages.
hebasto commented at 10:44 AM on June 1, 2020: member

Concept ACK.
jonathanschoeller force-pushed on Jun 1, 2020

in src/init.cpp:983 in b546870d17 outdated

 981 |  
 982 |      // Warn if unrecognized section name are present in the config file.
 983 | +    bilingual_str warnings;
 984 |      for (const auto& section : gArgs.GetUnrecognizedSections()) {
 985 | -        InitWarning(strprintf(Untranslated("%s:%i ") + _("Section [%s] is not recognized."), section.m_file, section.m_line, section.m_name));
 986 | +        warnings += strprintf(Untranslated("%s:%i ") + _("Section [%s] is not recognized.\n"), section.m_file, section.m_line, section.m_name);

MarcoFalke commented at 11:24 AM on June 1, 2020:

please don't change the translation string _("Section [%s] is not recognized.") and keep the special character \n untranslated (like above)

hebasto commented at 11:26 AM on June 1, 2020:

        warnings += strprintf(Untranslated("%s:%i ") + _("Section [%s] is not recognized.") + Untranslated("\n"), section.m_file, section.m_line, section.m_name);

hebasto commented at 11:28 AM on June 1, 2020:

Slow typing from my side :)

hebasto changes_requested

hebasto commented at 11:26 AM on June 1, 2020: member

ACK b546870d178222acd5d3f131410b8846f4875412, tested on Linux Mint 19.3 (x86_64).

I suggest to add a commit to modify configure.ac as well:

--- a/configure.ac
+++ b/configure.ac
@@ -391,6 +391,7 @@ if test "x$enable_werror" = "xyes"; then
   dnl https://gcc.gnu.org/bugzilla/show_bug.cgi?id=78010
   AX_CHECK_COMPILE_FLAG([-Werror=suggest-override],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=suggest-override"],,[[$CXXFLAG_WERROR]],
                         [AC_LANG_SOURCE([[struct A { virtual void f(); }; struct B : A { void f() final; };]])])
+  AX_CHECK_COMPILE_FLAG([-Werror=unreachable-code-loop-increment],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=unreachable-code-loop-increment"],,[[$CXXFLAG_WERROR]])
 fi
 
 if test "x$CXXFLAGS_overridden" = "xno"; then
@@ -410,6 +411,7 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
   AX_CHECK_COMPILE_FLAG([-Wsign-compare],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wsign-compare"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wsuggest-override],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wsuggest-override"],,[[$CXXFLAG_WERROR]],
                         [AC_LANG_SOURCE([[struct A { virtual void f(); }; struct B : A { void f() final; };]])])
+  AX_CHECK_COMPILE_FLAG([-Wunreachable-code-loop-increment],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wunreachable-code-loop-increment"],,[[$CXXFLAG_WERROR]])
 
   dnl Some compilers (gcc) ignore unknown -Wno-* options, but warn about all
   dnl unknown options if any other warning is produced. Test the -Wfoo case, and

refactor: Fix unreachable code in init arg checks

Building with -Wunreachable-code-loop-increment causes a warning
due to always returning on the first iteration of the loop that
outputs errors on invalid args.

Collect all errors, and output them in a single error message
after the loop completes, resolving the warning and avoiding
popup hell by outputting a seperate message for each error.

d15db4b1fc

build: Enable unreachable-code-loop-increment

Enable unreachable-code-loop-increment and treat as error.
refs: #19015

eea8114657

jonathanschoeller force-pushed on Jun 1, 2020
jonathanschoeller commented at 12:35 AM on June 2, 2020: none

Thanks @practicalswift , @MarcoFalke , and @hebasto for welcoming me and taking the time to help me with this PR.
DrahtBot commented at 1:28 AM on June 2, 2020: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #18857 (build: avoid repetitions when enabling warnings in configure.ac by vasild)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
hebasto approved
hebasto commented at 6:15 AM on June 2, 2020: member

re-ACK eea81146571480b2acd12c8cd7f36b04d056c47f, only suggested changes applied since the previous review.
laanwj commented at 2:27 PM on June 4, 2020: member

Code review ACK eea81146571480b2acd12c8cd7f36b04d056c47f
laanwj merged this on Jun 4, 2020
laanwj closed this on Jun 4, 2020
jonathanschoeller deleted the branch on Jun 4, 2020
sidhujag referenced this in commit 2ab90a2fe4 on Jun 4, 2020
deadalnix referenced this in commit 6c84fca7a2 on Dec 10, 2020
deadalnix referenced this in commit e27d9b655a on Dec 11, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#19015 build: Enable some commonly enabled compiler diagnostics #19017 Fix -Wunreachable-code-loop-increment warning

refactor: Fix unreachable code in init arg checks #19131

Conflicts