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

Chore: Put the three doc strings about each op together #5708

Merged
merged 2 commits into from
Aug 25, 2023

Conversation

jannotti
Copy link
Contributor

This PR should not affect any run-time behavior, nor any generated specs. It simply puts all of the documentation for an opcode together in one place, so it's slightly easier to add opcodes and find all of the documentation about an opcode in the source.

An eventual further change might be to get the documentaion into the OpSpecs themselves, as there is still a little confusion there - immediates are named in the OpSpec, but documented in doc.go.

The "test plan" here is really just that the generated specs are unchanged.

@jannotti jannotti changed the title Put the three doc strings about each op together Chore: Put the three doc strings about each op together Aug 24, 2023
@jannotti jannotti self-assigned this Aug 24, 2023
This PR should not affect any run-time behavior, nor any generated
specs. It simply puts all of the documentation for an opcode together
in one place, so it's slightly easier to add opcodes and find all of
the documentation about an opcode in the source.

An eventual further change might be to get the documentaion into the
OpSpecs themselves, as there is still a little confusion there -
immediates are _named_ in the OpSpec, but documented in doc.go.
@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #5708 (682bbb2) into master (51f5314) will decrease coverage by 0.69%.
Report is 1 commits behind head on master.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master    #5708      +/-   ##
==========================================
- Coverage   55.38%   54.69%   -0.69%     
==========================================
  Files         466      466              
  Lines       65734    65734              
==========================================
- Hits        36407    35955     -452     
- Misses      26877    27293     +416     
- Partials     2450     2486      +36     
Files Changed Coverage Δ
data/transactions/logic/assembler.go 93.98% <60.00%> (-0.11%) ⬇️
data/transactions/logic/doc.go 75.00% <100.00%> (ø)

... and 51 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jannotti jannotti marked this pull request as ready for review August 24, 2023 16:18
ahangsu
ahangsu previously approved these changes Aug 24, 2023
Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

This PR essentially merge following 3 things:

  1. opDocByName
  2. opDocExtras
  3. opcodeImmediateNotes

into one map of OpDesc.

Removed the code path that allows pseudo-ops to execute non-existant
opcodes.  Changed tests to not test this path, and instead test that
the pseudo-op annotation was properly placed on real opcode.
Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Makes sense

@jannotti jannotti merged commit 572d728 into algorand:master Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants