Repository: bitcoin/bitcoin (language: cpp)

Issue:
Redundant code or latent bug
<!-- This issue tracker is only for technical issues related to Bitcoin Core.

General bitcoin questions and/or support requests are best directed to the Bitcoin StackExchange at https://bitcoin.stackexchange.com.

For reporting security issues, please read instructions at https://bitcoincore.org/en/contact/.

If the node is "stuck" during sync or giving "block checksum mismatch" errors, please ensure your hardware is stable by running memtest and observe CPU temperature with a load-test tool such as linpack before creating an issue! -->

<!-- Describe the issue -->
Looking at the code below I can't understand how it is possible for the IF statement conditional expression to become true. cnt is already incremented before the IF statement. Is this not redundant? https://github.com/bitcoin/bitcoin/blob/master/src/script/script.h#L594

I have opened a ticket, https://stackoverflow.com/questions/73088518/reduntant-code-possibly-in-bitcoin-script-h

**Expected behavior**
Possibly doesn't change the behavior. 
 

<!--- What behavior did you expect? -->
Not Applicable
**Actual behavior**

<!--- What was the actual behavior (provide screenshots if the issue is GUI-related)? -->

**To reproduce**

<!--- How reliably can you reproduce the issue, what are the steps to do so? -->
It is source code issue

**System information**
Not applicable 

<!-- What version of Bitcoin Core are you using, where did you get it (website, self-compiled, etc)? -->
Main branch ..https://github.com/bitcoin/bitcoin
<!-- What type of machine are you observing the error on (OS/CPU and disk type)? -->
Not Applicable

<!-- GUI-related issue? What is your operating system and its version? If Linux, what is your desktop environment and graphical shell? -->
Not Applicable
<!-- Any extra information that might be useful in the debugging process. -->
<!--- This is normally the contents of a `debug.log` or `config.log` file. Raw text or a link to a pastebin type site are preferred. -->



Discussion / hints:
@janus you are correct, but possibly this is not the final version of this function, since this is fairly recently added code and I think the work on Miniscript is still in progress.

CC: @darosior 
Thanks for the ping Fabian.

Janus, you are correct. My bad, i mistakenly moved it there [while fixing a previous bug with `cnt`](https://github.com/sipa/miniscript/pull/95#issuecomment-1026669775). The intention here is to mimic `ret.empty()`, so moving it after the condition makes this small optimization (moving instead of inserting) possible:
```diff
diff --git a/src/script/script.h b/src/script/script.h
index 3b799ad637..1e5f694d52 100644
--- a/src/script/script.h
+++ b/src/script/script.h
@@ -588,7 +588,6 @@ CScript BuildScript(Ts&&... inputs)
     int cnt{0};
 
     ([&ret, &cnt] (Ts&& input) {
-        cnt++;
         if constexpr (std::is_same_v<std::remove_cv_t<std::remove_reference_t<Ts>>, CScript>) {
             // If it is a CScript, extend ret with it. Move or copy the first element instead.
             if (cnt == 0) {
@@ -600,6 +599,7 @@ CScript BuildScript(Ts&&... inputs)
             // Otherwise invoke CScript::operator<<.
             ret << input;
         }
+        cnt++;
     } (std::forward<Ts>(inputs)), ...);
 
     return ret;
```

I'll open a PR with this patch.
> Thanks for the ping Fabian.
> 
> Janus, you are correct. My bad, i mistakenly moved it there [while fixing a previous bug with `cnt`](https://github.com/sipa/miniscript/pull/95#issuecomment-1026669775). The intention here is to mimic `ret.empty()`, so moving it after the condition makes this small optimization (moving instead of inserting) possible:
> 
> ```diff
> diff --git a/src/script/script.h b/src/script/script.h
> index 3b799ad637..1e5f694d52 100644
> --- a/src/script/script.h
> +++ b/src/script/script.h
> @@ -588,7 +588,6 @@ CScript BuildScript(Ts&&... inputs)
>      int cnt{0};
>  
>      ([&ret, &cnt] (Ts&& input) {
> -        cnt++;
>          if constexpr (std::is_same_v<std::remove_cv_t<std::remove_reference_t<Ts>>, CScript>) {
>              // If it is a CScript, extend ret with it. Move or copy the first element instead.
>              if (cnt == 0) {
> @@ -600,6 +599,7 @@ CScript BuildScript(Ts&&... inputs)
>              // Otherwise invoke CScript::operator<<.
>              ret << input;
>          }
> +        cnt++;
>      } (std::forward<Ts>(inputs)), ...);
>  
>      return ret;
> ```
> 
> I'll open a PR with this patch.
@fjahr @darosior Thanks for your comments. 

I hope to start contributing patches in the future. 


Fix patch:
diff --git a/src/script/script.h b/src/script/script.h
index 3b799ad63774e..1e5f694d528df 100644
--- a/src/script/script.h
+++ b/src/script/script.h
@@ -588,7 +588,6 @@ CScript BuildScript(Ts&&... inputs)
     int cnt{0};
 
     ([&ret, &cnt] (Ts&& input) {
-        cnt++;
         if constexpr (std::is_same_v<std::remove_cv_t<std::remove_reference_t<Ts>>, CScript>) {
             // If it is a CScript, extend ret with it. Move or copy the first element instead.
             if (cnt == 0) {
@@ -600,6 +599,7 @@ CScript BuildScript(Ts&&... inputs)
             // Otherwise invoke CScript::operator<<.
             ret << input;
         }
+        cnt++;
     } (std::forward<Ts>(inputs)), ...);
 
     return ret;


Test patch: