-
Notifications
You must be signed in to change notification settings - Fork 13
Remove type_check from Property trait
#74
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
Conversation
type_check from Property trait
65b55f7 to
f2d6835
Compare
|
concept ACK based on your description. But the CI failures are real; the fuzztests need to be updated. |
The blanket implementation of `type_check` is only used on `CompilerExtData` while `ExtData` and `Type` override the impl and don't need the `child` parameter. Moreover, the fn isn't called as Property generic. So the blanket implementation of `type_check` is moved as a impl in `CompilerExtData` and the overrides in `ExtData` and `Type` are moved as simple impl on the type, making it possible to remove the unused `child` parameter.
f2d6835 to
500b4a5
Compare
The issue was that I removed the the error with the closure was: The compiler error is fixed but I still don't grasp what's going on :S |
|
Ci is fixed, does this need something? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 500b4a5
5f41cb6 change FnMut -> Fn (Riccardo Casatta) cdcd53e Refactor out type_check (Riccardo Casatta) Pull request description: This apply the same logic of ElementsProject/elements-miniscript#74 removing a couple of unreachable, simplify things and very likely reduce binary bloat. ~~However, in doing so, I don't think this MR is equivalent to master for the logic used for `get_child`.~~ ~~I suspect current master is broken and this fix it (or viceversa?), but I have to suspend this work for now.~~ ACKs for top commit: apoelstra: ACK 5f41cb6 this is great, thanks! Tree-SHA512: 4b904fde55ba75895d377babde5e7a3a215cdf3b0ccff0c90572bb286de9d8a80234fc6a4921ad83bcb8c947affa1e3c23b2135534cb4363f38f1bab89368ba4
5f41cb64b5927dcd06b0c1ecda23ed27518b16d9 change FnMut -> Fn (Riccardo Casatta) cdcd53e36dc444d95291409b733604cde12faf9b Refactor out type_check (Riccardo Casatta) Pull request description: This apply the same logic of ElementsProject/elements-miniscript#74 removing a couple of unreachable, simplify things and very likely reduce binary bloat. ~~However, in doing so, I don't think this MR is equivalent to master for the logic used for `get_child`.~~ ~~I suspect current master is broken and this fix it (or viceversa?), but I have to suspend this work for now.~~ ACKs for top commit: apoelstra: ACK 5f41cb64b5927dcd06b0c1ecda23ed27518b16d9 this is great, thanks! Tree-SHA512: 4b904fde55ba75895d377babde5e7a3a215cdf3b0ccff0c90572bb286de9d8a80234fc6a4921ad83bcb8c947affa1e3c23b2135534cb4363f38f1bab89368ba4
I was looking to port rust-bitcoin/rust-miniscript#584 here, but I realized we can probably do better (and maybe port this in rust-miniscript to simplify things)
The blanket implementation of
type_checkis only used onCompilerExtDatawhileExtDataandTypeoverride the impl and don't need thechildparameter. Moreover, the fn isn't called as Property generic.So the blanket implementation of
type_checkis moved as a impl inCompilerExtDataand the overrides inExtDataandTypeare moved as simple impl on the type, making it possible to remove the unusedchildparameter.