refactor: Fix unreachable code in init arg checks by jonathanschoeller · Pull Request #19131 · bitcoin/bitcoin

jonathanschoeller · 2020-06-01T08:25:41Z

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.

bitcoin/src/init.cpp

Lines 968 to 972 in aa8d768

    
           // on the command line or in this network's section of the config file. 
        
           std::string network = gArgs.GetChainName(); 
        
           for (const auto& arg : gArgs.GetUnsuitableSectionOnlyArgs()) { 
        
               return InitError(strprintf(_("Config setting for %s only applied on %s network when in [%s] section."), arg, network, network)); 
        
           }

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.

practicalswift · 2020-06-01T09:08:14Z

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! :)

maflcko

Nice. ACK with or without the nit addressed

src/init.cpp

maflcko · 2020-06-01T10:35:32Z

Oh, it looks like you might have to adjust the tests for the longer error messages.

hebasto · 2020-06-01T10:44:52Z

Concept ACK.

maflcko · 2020-06-01T11:24:48Z

src/init.cpp

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

Slow typing from my side :)

hebasto

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

hebasto · 2020-06-01T11:26:39Z

src/init.cpp

Suggested change

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

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

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.

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

jonathanschoeller · 2020-06-02T00:35:28Z

Thanks @practicalswift , @MarcoFalke , and @hebasto for welcoming me and taking the time to help me with this PR.

DrahtBot · 2020-06-02T01:28:19Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

build: avoid repetitions when enabling warnings in configure.ac #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

re-ACK eea8114, only suggested changes applied since the previous review.

laanwj · 2020-06-04T14:27:28Z

Code review ACK eea8114

eea8114 build: Enable unreachable-code-loop-increment (Jonathan Schoeller) d15db4b refactor: Fix unreachable code in init arg checks (Jonathan Schoeller) Pull request description: Closes: bitcoin#19017 In bitcoin#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. ```shell 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. ACKs for top commit: laanwj: Code review ACK eea8114 hebasto: re-ACK eea8114, only suggested changes applied since the [previous](bitcoin#19131 (review)) review. Tree-SHA512: 2aa3ceb7fab581b6ba2580900668388d8eba1c3001c8ff9c11c1f4a9a10fbc37f30e590249862676858446e3f4950140a252953ba1643ba3bfd772f8eae20583

Summary: Backport of core [[bitcoin/bitcoin#19131 | PR19131]]. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8648

Summary: Follow-up for rABC6c84fca7a26eee3f5f031f02610d4528f7ef8276 which lost the changes for an unknown reason. Backport of core [[bitcoin/bitcoin#19131 | PR19131]]. Test Plan: With Werror enabled: ninja all check-all Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D8658

fanquake added the Refactoring label Jun 1, 2020

maflcko approved these changes Jun 1, 2020

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

jonathanschoeller force-pushed the fix-Wunreachable-code-loop-increment branch from 34c4fc9 to b546870 Compare June 1, 2020 10:55

maflcko reviewed Jun 1, 2020

View reviewed changes

hebasto requested changes Jun 1, 2020

View reviewed changes

jonathan-schoeller-rea added 2 commits June 2, 2020 06:20

build: Enable unreachable-code-loop-increment

eea8114

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

jonathanschoeller force-pushed the fix-Wunreachable-code-loop-increment branch from b546870 to eea8114 Compare June 1, 2020 20:36

DrahtBot mentioned this pull request Jun 2, 2020

build: avoid repetitions when enabling warnings in configure.ac #18857

Closed

hebasto approved these changes Jun 2, 2020

View reviewed changes

laanwj merged commit b46fb5c into bitcoin:master Jun 4, 2020

jonathanschoeller deleted the fix-Wunreachable-code-loop-increment branch June 4, 2020 21:06

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

	// on the command line or in this network's section of the config file.
	std::string network = gArgs.GetChainName();
	for (const auto& arg : gArgs.GetUnsuitableSectionOnlyArgs()) {
	return InitError(strprintf(_("Config setting for %s only applied on %s network when in [%s] section."), arg, network, network));
	}

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

Conversation

jonathanschoeller commented Jun 1, 2020

Uh oh!

practicalswift commented Jun 1, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jun 1, 2020

Uh oh!

hebasto commented Jun 1, 2020

Uh oh!

maflcko Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Jun 1, 2020

Choose a reason for hiding this comment

Uh oh!

jonathanschoeller commented Jun 2, 2020

Uh oh!

DrahtBot commented Jun 2, 2020

Conflicts

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jun 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

hebasto left a comment •

edited

Loading