Implement postprocessors using traits #184
Open
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
These are like the Postprocessor callback function type, but they can be implemented on types for more ergonomic stateful postprocessing. Fixes zoni#175
This creates a parallel EmbedPostprocessor trait, and it adds new methods for registering trait objects into the postprocessor chains. The trait is implemented for the callback Fn type that was previously declared as the Postprocessor type.
rsesek
force-pushed
the
postprocessor-improvements
branch
from
338867c to
1fcdd9d
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.
These are like the Postprocessor callback function type, but they can be implemented on types for more ergonomic stateful postprocessing.
Fixes #175
I've structured this as two commits:
traitdefinitions and uses anenumto store the postprocessorspubdefinition ofPostprocessorfrom thetypeto atraitIt is possible to avoid introducing a second set of methods on
Exporter, and the code could be changed so thatadd_postprocessorjust takes a&'a dyn Postprocessorand theadd_*_implmethods wouldn’t need to be added. However, that breaks some existing callers that use closures without typed arguments. It seems the compiler cannot infer the closure argument types when implicitly converting between the closure and theFnpointer.I tried to make it so that the existing
add_postprocesormethod could take an impl as well as continuing to take the callback type. But I couldn’t get that to work while also still allowing concise closure syntax with omitting the parameter types (it seems that for the closure type inference to work, every closure would need to fully specify the callback type signature).Opening this PR more as a discussion point, and I understand if this isn’t something you want to add. With the other commit for #175, this is more just an API ergonomics enhancement.