Skip to content

refactor: Make CScriptVisitor stateless #18863

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
maflcko merged 1 commit into from
Jun 19, 2020
Merged

refactor: Make CScriptVisitor stateless #18863

maflcko merged 1 commit into from
Jun 19, 2020

Conversation

@promag
Copy link
Contributor

@promag promag commented May 4, 2020 •
edited
Loading

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

@promag
Copy link
Contributor Author

promag commented May 4, 2020

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

@maflcko
Copy link
Member

maflcko commented May 4, 2020

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.

promag
promag commented May 4, 2020
View reviewed changes
@maflcko
Copy link
Member

maflcko commented May 4, 2020

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

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

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

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUichwwAgiZLmgANz5JYgG5YI/UqYLczqAGB1HgiAvykODXF0Ew7PowJt9DFLJE0
lM6Rfzy9n9086xvqItRf3jBH3l2IJ0Zp4AeSCUQV0t7s6sT3+vyqr7G6y1S+m8ai
0RjiSADrutAyd69XY2TFcrwLWGGeA4Zk+JMt4dcUGVaGA1T2IveNkeBwG6N0TtVf
I2DwYidq5Q620EGdchwrbkGInoXOoygBZ2Cths7MrNsaGl6XXOInJ6eM4azIqz04
why88n1FCcFq0cakKbuQnzf7Llx6ZTs2X1lbPM2t8PfYa1jTjb6zC0xkW72si4tj
BSps4CwmQb6bFpB7A2p/dTc6Int7+xubjlhRTXO6m3KcYvYeKBjdWzDhbxhCsgY9
Q+FJGJ3pCT/aENy8OeYg+erxgl94bse/x75mALfDVoXqqOAYyI8Qg0Dvk2nYnMhL
FC60WPXNqM9agXrNcWZdHzYWFcXoT1YzcReWHf/Whk4zT3iT2FGmVUKNh/HCvavs
X90+LGrb
=GIW/
-----END PGP SIGNATURE-----

Timestamp of file with hash 9c646a620b75db224cc45898abb4e8eea4d622bbe4c774fba01d191da0fd0bde -

@practicalswift
Copy link
Contributor

practicalswift commented May 4, 2020

Concept ACK

@maflcko
Copy link
Member

maflcko commented May 4, 2020

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

@maflcko maflcko closed this May 4, 2020
@maflcko maflcko reopened this May 4, 2020
morucci added a commit to change-metrics/monocle that referenced this pull request May 10, 2020
@morucci
api, ui: Fix wrong abandoned ratio + count
7947350 
When a change is closed and re-opened then the compute of changes
opened + merged + abandoned != changes created.

It was due to the compute method that was based on the events but not
on the status of the change.

Example of change: bitcoin/bitcoin#18863
@morucci morucci mentioned this pull request May 10, 2020
Merged
@maflcko maflcko mentioned this pull request May 13, 2020
Closed
@promag
Copy link
Contributor Author

promag commented May 24, 2020

@sipa friendly ping.

@sipa
Copy link
Member

sipa commented May 25, 2020

utACK ae0891e87b0aa0ce57a77dbd99555803a539d6ea

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

@promag promag force-pushed the 2020-04-cscript-visitor branch from ae0891e to 2102f20 Compare May 25, 2020 08:36
@promag
Copy link
Contributor Author

promag commented May 25, 2020

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

Done.

@promag promag force-pushed the 2020-04-cscript-visitor branch from 2102f20 to 1eb7393 Compare May 25, 2020 10:32
@maflcko
Copy link
Member

maflcko commented May 26, 2020

ACK 1eb73933399dc8d9ff536fa668a98b03bd7d597b 🎰

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 1eb73933399dc8d9ff536fa668a98b03bd7d597b 🎰
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhSiQv/fgvq0c6RiAaVCFdQYJEw9rluU+T8RqlpmWsEO6X7Bv/1N27halQrg91Y
x0EzilW+nS+FUgMYfnm+QEU49Q7Tb1BgZa2xR6lUueS+koV01FzKJf1niDa6D9ll
CLvvQP10Cc02c4QZLM7vfrrIEHPRbzD+Nl5nPbRDMDwfGshsMOcfhV7t/7mI718i
BssPWXqiCPPbLu7TZIiWwSZjcj3ISkbrfiPvpBuWkCQ11n5Da2C4vubcIY8D5uIZ
fvH9s/Tp+Uu6VgyNWmwLfs9Mhq1HiuMxdoIWnXwz8XTp7xZgf+THEAfDbpoPQty0
Nt4zH8bokN4jxVM9Ac162fZs3hw4jxqTS0zpjzDqp4YGun1CDkNgm/AVciL6UFoK
aoXcBJmTe8NE24FGTpErtUIIVIdY+yf1oex8SkjzkCZ0i7v8HnlucdTjsmS/Za6K
NkesEzQXgq0A7zBcsbtVlO29QgoxULvPIXxr/evnXI3JOYdsRvGlv+ibAVtdlgWy
GwUy9f8h
=OqF+
-----END PGP SIGNATURE-----

Timestamp of file with hash d8f829f43c62cd26aff2b38a635ed9ae65308f0b2c3a17cbf8bbb9470285b962 -

@maflcko maflcko mentioned this pull request Jun 5, 2020
Closed
@sipa
Copy link
Member

sipa commented Jun 5, 2020

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.

@promag promag force-pushed the 2020-04-cscript-visitor branch from 1eb7393 to 3351c91 Compare June 5, 2020 23:42
@promag
Copy link
Contributor Author

promag commented Jun 5, 2020

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

@promag promag changed the title refactor: Drop unused CScriptVisitor return type refactor: Make CScriptVisitor stateless Jun 5, 2020
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 6, 2020 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@promag
Copy link
Contributor Author

promag commented Jun 15, 2020

@MarcoFalke @sipa should be an easy review.

@DrahtBot DrahtBot mentioned this pull request Jun 16, 2020
Closed
5 tasks
@maflcko
Copy link
Member

maflcko commented Jun 17, 2020

ACK 3351c91 🏤

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 3351c91ed402895dcb4f803a29d2cac70ccfa8b4 🏤
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUg8ygv/V2sqtDYeTS42806JVJrqjvyeXKqMUmYa9wTTJSt/Tqg5VwAf1P3NEwnW
pFdpfLWpJwrgFCIxO2yNZP1cwTY7JNYogFr8A223YuWVcKcr/nRibKr5IbY+jCcu
/TDA/t01fOWwtYr9hBC/g5I5nxjnAUR354f/gcHVYeXso3UnKBCyTim74Ddz9SKb
7UAsMM0rQ0VNvl0RgocrjtpyeE0V8ilbZI3y36bsDTlD1v1TSHyyyszC2zSobQ6b
P1kt24Ig1qcLKy4HmFPq973AVXeksNDlG43WzXzLX9AV8I89G/u/0Nd8hzcki+yc
IzcdBAMa6FZkGOZUPGKIFmmuUht4eDXXtd9gzwJDY9BzLZoZYp9PgNwPYSoWGzBr
M3CTky/KyxbHcMZ6HXViRc8Bg3kdfEcz8TteJQ45fYCLJztq+wTGgnOiyETaEJHt
BnMCvloMScQDophG75v7hU9W6LaoqjSIQukRUZ7HF6k5E6C0YKRfGWU/TuRygdp8
++LNdF8H
=gDWo
-----END PGP SIGNATURE-----

Timestamp of file with hash 61e0aed6678c57e234d6fe4ca5ad6495ad6a11f3d4a972801fd6dc65a7737a60 -

@promag
Copy link
Contributor Author

promag commented Jun 18, 2020

@sipa ping.

@sipa
Copy link
Member

sipa commented Jun 18, 2020

utACK 3351c91

maflcko
maflcko reviewed Jun 19, 2020
View reviewed changes
src/script/standard.cpp
}
};

const CScriptVisitor g_script_visitor;
Copy link
Member

@maflcko maflcko Jun 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@maflcko maflcko merged commit 5f72ddb into bitcoin:master Jun 19, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 7, 2020
Merge bitcoin#18863: refactor: Make CScriptVisitor stateless
bff30e5 
3351c91 refactor: Make CScriptVisitor stateless (João Barbosa)

Pull request description:

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

ACKs for top commit:
  MarcoFalke:
    ACK 3351c91 🏤
  sipa:
    utACK 3351c91

Tree-SHA512: d158ad2ebe8ea4dc8cc090b943dd66fa5421a84f9443e16ab2d661df38e1a85de16ff13cbaa56924489d8d43cba25fa3cd8b6904bbbcbf356b886ffe8ffba19a
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Aug 26, 2021
@promag @PiRK
refactor: Make CScriptVisitor stateless
a112f4d 
Summary:

> CScriptVisitor was added in [[bitcoin/bitcoin#1357 | core#1357]] and the visitor return type was never used. Now CScriptVisitor is stateless and CScript is the return type.

This is a backport of [[bitcoin/bitcoin#18863 | core#18863]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D9939
kwvg added a commit to kwvg/dash that referenced this pull request Sep 15, 2021
@kwvg
merge bitcoin#18863: Make CScriptVisitor stateless
c60491e
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@promag @maflcko @practicalswift @sipa @DrahtBot