Skip to content

Generate contents of decodePrimitive automatically#278

Merged
roconnor-blockstream merged 1 commit intomasterfrom
auto-decodePrimitive
Jan 7, 2025
Merged

Generate contents of decodePrimitive automatically#278
roconnor-blockstream merged 1 commit intomasterfrom
auto-decodePrimitive

Conversation

@roconnor-blockstream
Copy link
Collaborator

The new generation code isn't exactly the same as the orginal code. There is no optimistic lifting out of a seconde decoding (code2 variable). Such optimizations could be added back in if wanted, but it is probably best to keep the code straightforward.

The decodePrimitive is already being tested by the "Serialization.C" tests in the Haskell test suite via FFI bindings.

The new generation code isn't exactly the same as the orginal code.
There is no optimistic lifting out of a seconde decoding (code2 variable).
Such optimizations could be added back in if wanted, but it is probably best to
keep the code straightforward.

The decodePrimitive is already being tested by the "Serialization.C" tests in the
Haskell test suite via FFI bindings.
@roconnor-blockstream
Copy link
Collaborator Author

Fixes #195.

@apoelstra you are going to need to update CI to run GenDecodeJet to make sure the content of the repo matches what is checked in.

As noted above, when the generated code is not identical to the original code. I suggest something like git diff master --color-moved --color-moved-ws=allow-indentation-change when reviewing the changes.

Copy link
Contributor

@uncomputable uncomputable left a comment

Choose a reason for hiding this comment

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

ACK de799bd I manually compared the old and new files and they seem to do the same thing.

@apoelstra
Copy link
Collaborator

git diff master --color-moved --color-moved-ws=allow-indentation-change

I'll see what I can do. Inside the nix sandbox I don't have the git repo. Though of course I can run git init and git add and get access to git-diff that way.

Having said that, checking this by hand I see:

  • decodeCoreJets.inc exactly matches C/decodeCoreJets.inc
  • decodeElementsJets.inc does not appear anywhere in the repo. Is this supposed to be committed?

@uncomputable
Copy link
Contributor

uncomputable commented Jan 5, 2025

@apoelstra decodeElementsJets.inc lives in C/primitive/elements, no?

@apoelstra
Copy link
Collaborator

Oh, yes, sorry, I was invoking find incorrectly.

Copy link
Collaborator

@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 de799bd; successfully ran local tests

@apoelstra
Copy link
Collaborator

Updated my local CI with an exact diff check. If this starts failing I will explore ways to make it more flexible.

@roconnor-blockstream
Copy link
Collaborator Author

Don't you already have your CI comparing the output of GenPrecompute and friends?

@roconnor-blockstream roconnor-blockstream merged commit de799bd into master Jan 7, 2025
@roconnor-blockstream roconnor-blockstream deleted the auto-decodePrimitive branch January 7, 2025 15:53
@apoelstra
Copy link
Collaborator

@roconnor-blockstream yes.

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.

3 participants