Remove recursion in semantic::Policy::normalized
#614
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tcharding
force-pushed
the
10-12-rm-recursion-normalized
branch
from
db50bf1
to
333682e
Compare
|
Fuzz fail looks real, this must be buggy. Converting to draft. |
Remove unnecessary code comment, the local variables names are clear enough.
This function is fiendishly hard to understand, add a line of whitespace as a tiny baby step towards making it clearer.
Currently we are using `map_or` in a convoluted manner, we can just use `unwrap_or` to get the inner value or default to 0. Refactor only, no logic changes.
We have a little typo in an error string, fix it up.
Use standard from for import statements. (Note `Arc` is already in scope in `super`.)
When we implemented the `TreeLike` trait for the concrete `Policy` we put it in the `iter` module, it doesn't really live there, better to put the impl in the same place as the definition of the implementor. Move the impl of `TreeLike` for `concrete::Policy` to the `concrete` module.
In preparation for removing recursive algorithms in the `semantic` module add an `Arc` wrapper around the policies in the `Thresh` vector. Note we use the more explicit `Arc::new` instead of `into` because a lot of these should be removed when we remove recursion, later it may be necessary to check that we are not creating new references when a clone will suffice, the explicit `Arc::new` is easier to check. In tests however just use `into`.
In preparation for removing recursive algorithms in the `semantic` module implement the `TreeLike` trait to enable iteration over policy nodes. This is a direct copy of the `concrete::Policy` impl with the `And` and `Or` variants removed.
Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::for_each_key`.
Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::translate_pk`.
Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::n_terminals`.
Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in the `semantic::Policy::real_*_timelocks` functions. Done together because they are basically identical.
Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::at_age` and `semantic::Policy::at_age`. Done together because they are basically identical.
Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::n_keys`.
Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::minimum_n_keys`.
Done as part of the effort to remove all the recursion crate wide. Use the `TreeLike` trait to iterate over policy nodes and remove the recursive call in `semantic::Policy::sorted`.
The `semantic::Policy::normalized` function is recursive and has nested processing of `Threshold`s. It is not trivial to fully grok. The function is already tested elsewhere in the unit tests but we don't have simple single purpose tests for it. Add various unit tests for `normalized`, these serve as documentation of the function as much as anything.
Currently we are creating a new `Arc<Policy::Threshold>` from the pieces of `sub` that we pattern matched on. We can just clone the original `sub` instead.
In `normalized` we process the threshold policy as well as its child threshold policy and we shadow `subs` - this is all very confusing. Add parent/child terminology and a couple extra local variables in an attempt to make the code more clear. Refactor only, no logic changes.
The `normalize` function spans more than a screen (on my monitor), as such it is hard to work on. Refactor out the (m, n)-thresh calculation to a function so that the body of the function becomes shorter. Refactor only, no logic changes.
Remove the recursive call in `semantic::Policy::normalized`, using the `TreeLike` trait to iterate nodes instead.
tcharding
force-pushed
the
10-12-rm-recursion-normalized
branch
from
333682e
to
1f0486b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The last 5 patches, the rest is from #612
Make sure you have a cup of tea to review this one, its pretty wild - took all of my brain power to work this out.