refactor: Make CScriptVisitor stateless #18863

promag commented at 10:06 AM on May 4, 2020: member

CScriptVisitor was added in 1025440184ef100a22d07c7bb543ee45cf169d64 (#1357) and the visitor return type was never used. Now CScriptVisitor is stateless and CScript is the return type.

promag commented at 10:07 AM on May 4, 2020: member

@sipa mind taking a look since you wrote this code?

MarcoFalke commented at 11:16 AM on May 4, 2020: member

All of this code will go away anyway when we switch to C++17 in 5 months, except for the script << .. parts.

ACK on changing * to &, since that will be needed for my patch to switch to C++17. No opinion on changing the return type.

in src/script/standard.cpp:251 in ae0891e87b outdated

 249 | -    explicit CScriptVisitor(CScript *scriptin) { script = scriptin; }
 250 | +    explicit CScriptVisitor(CScript& scriptin) : script(scriptin) {}
 251 |  
 252 | -    bool operator()(const CNoDestination &dest) const {
 253 | -        script->clear();
 254 | -        return false;

promag commented at 11:19 AM on May 4, 2020:

I'm curious why return false here.

MarcoFalke commented at 11:36 AM on May 4, 2020:

Sometimes code is written to have a return type even though the return type is initially unused. Motivation for that could be that the initial implementation wants to accommodate future callers of the code, which may want to use the return type.

promag commented at 11:52 AM on May 4, 2020:

Yeah, I think that motivation is long gone.

DrahtBot added the label Refactoring on May 4, 2020

MarcoFalke commented at 11:58 AM on May 4, 2020: member

ACK ae0891e87b0aa0ce57a77dbd99555803a539d6ea, reviewed first commit with --word-diff and second commit with --word-diff-regex=. 🌲

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 9c646a620b75db224cc45898abb4e8eea4d622bbe4c774fba01d191da0fd0bde -

</details>

practicalswift commented at 2:08 PM on May 4, 2020: contributor

Concept ACK

MarcoFalke commented at 3:08 PM on May 4, 2020: member

Open-Close to re-run ci. See #15847 (comment)

MarcoFalke closed this on May 4, 2020

MarcoFalke reopened this on May 4, 2020

morucci referenced this in commit 7947350645 on May 10, 2020

promag commented at 11:59 PM on May 24, 2020: member

@sipa friendly ping.

sipa commented at 6:23 AM on May 25, 2020: member

utACK ae0891e87b0aa0ce57a77dbd99555803a539d6ea

It could be made even shorter using script = CScript() << ...; statements.

promag force-pushed on May 25, 2020

promag commented at 10:31 AM on May 25, 2020: member

It could be made even shorter using script = CScript() << ...; statements.

Done.

promag force-pushed on May 25, 2020

MarcoFalke commented at 11:42 AM on May 26, 2020: member

ACK 1eb73933399dc8d9ff536fa668a98b03bd7d597b 🎰

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash d8f829f43c62cd26aff2b38a635ed9ae65308f0b2c3a17cbf8bbb9470285b962 -

</details>

sipa commented at 11:00 PM on June 5, 2020: member

utACK the current code.

Another suggestion - and feel free to ignore - given that the return type bool is now gone, why not use it for the script itself? The return type could be CScript directly.

refactor: Make CScriptVisitor stateless 3351c91ed4

promag force-pushed on Jun 5, 2020

promag commented at 11:46 PM on June 5, 2020: member

Yeah I saw it too - previous change made it obvious - but was lazy to do it. Updated, looks better.

promag renamed this:
~~refactor: Drop unused CScriptVisitor return type~~
refactor: Make CScriptVisitor stateless
on Jun 5, 2020

DrahtBot commented at 4:26 AM on June 6, 2020: member

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

Conflicts

No conflicts as of last run.

promag commented at 2:21 PM on June 15, 2020: member

@MarcoFalke @sipa should be an easy review.

MarcoFalke commented at 10:47 AM on June 17, 2020: member

ACK 3351c91ed402895dcb4f803a29d2cac70ccfa8b4 🏤

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 61e0aed6678c57e234d6fe4ca5ad6495ad6a11f3d4a972801fd6dc65a7737a60 -

</details>

promag commented at 11:36 PM on June 18, 2020: member

@sipa ping.

sipa commented at 11:58 PM on June 18, 2020: member

utACK 3351c91ed402895dcb4f803a29d2cac70ccfa8b4

in src/script/standard.cpp:276 in 3351c91ed4

 299 | -        return true;
 300 | +        return CScript() << CScript::EncodeOP_N(id.version) << std::vector<unsigned char>(id.program, id.program + id.length);
 301 |      }
 302 |  };
 303 | +
 304 | +const CScriptVisitor g_script_visitor;

MarcoFalke commented at 11:54 AM on June 19, 2020:

nit: function-scope globals are better than anon-scope globals, but all this code will go away with C++17, so no big deal

MarcoFalke merged this on Jun 19, 2020

MarcoFalke closed this on Jun 19, 2020

sidhujag referenced this in commit bff30e54f0 on Jul 7, 2020

kittywhiskers referenced this in commit 5b9a4c1acd on Jul 15, 2021

kittywhiskers referenced this in commit 116b3e6b36 on Jul 17, 2021

Fabcien referenced this in commit a112f4d8d3 on Aug 26, 2021

kittywhiskers referenced this in commit 7faa169ba9 on Sep 11, 2021

kittywhiskers referenced this in commit c60491eef1 on Sep 15, 2021

thelazier referenced this in commit ca1f9b5756 on Sep 19, 2021

thelazier referenced this in commit acbbeef88e on Sep 25, 2021

DrahtBot locked this on Feb 15, 2022