Skip to content
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.

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

Rename opcodes::All to Opcode #1955

Merged
merged 4 commits into from Aug 11, 2023

Conversation

tcharding
Copy link
Member

I may be missing something about the All type but it seems it can be re-named to Opcode with no loss of clarity.

The first few patches do some other clean ups, the last patch is the meat and potatoes.

@apoelstra
Copy link
Member

This all looks good. The All name is a vestige of 2015-era pre-Miniscript code when I would parse script out using classify. Then all the pushes would become enum variants containing &[u8], all the OP_RETURN synonyms would become a single thing, and then all the "real" opcodes would go into the smaller Ordinary enum. And the All type was if I actually wanted all the opcodes.

Today I think classify is still used to separate pushes from the rest of a script (through the Script::instructions iterator), but pretty much all interaction with opcodes happens through the All type.

It might be interesting to look at how Script::instructions is actually used. It may be that we should drop the Class enum and Ordinary enum entirely, and just return a (All, Option<&[u8]>) tuple where the latter is populated for push opcodes.

Anyway concept ACK renaming All to Opcode.

apoelstra
apoelstra previously approved these changes Jul 26, 2023
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK ef9f049

Fix indentation of macro code and remove unnecessary line of whitespace.
This code comment is not commenting anything, remove it.
The opcode aliases are just that, aliases to opcodes so they can live in
the `all` module along with the other opcodes.
The `opcodes::All` type can seemingly be re-named to `Opcode` with no
loss of clarity - unless I'm missing something.
@tcharding
Copy link
Member Author

Rebased, no other changes.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK 63d0fa0

@tcharding
Copy link
Member Author

I believe this can merge, it has one ack two weeks old now, I would argue that it counts as a "trivial API change that no one really cares about" - except @stevenroose who asked me to do this change anyways (out of band).

@apoelstra apoelstra merged commit f1b3e69 into rust-bitcoin:master Aug 11, 2023
29 checks passed
@tcharding tcharding deleted the 07-26-rename-opcodes branch August 14, 2023 00:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants