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

add Composer implementation for reference types #231

Merged
merged 1 commit into from
Oct 18, 2023

Conversation

xofyarg
Copy link
Contributor

@xofyarg xofyarg commented Oct 9, 2023

The use case is to share the underlying Composer buffer between builders to avoid allocation, via a thread local variable for example. Without the reference type implements, the ownership could be transferred by memory swap operation, but that's not quite ideal. Plus some methods like MessageBuilder::from_target would drop the Composer on error, which makes it even difficult to share.

@partim
Copy link
Member

partim commented Oct 16, 2023

Could this just be a blanket impl for any mut ref to a composer? The compiler seems to accept that.

@xofyarg xofyarg force-pushed the add-composer-impls branch 2 times, most recently from 82aaf36 to b657496 Compare October 16, 2023 17:23
@xofyarg
Copy link
Contributor Author

xofyarg commented Oct 16, 2023

Even better. The PR has been updated.

@partim
Copy link
Member

partim commented Oct 17, 2023

This isn’t quite right – the trait functions should defer to the functions in the referenced type and not use the default impls. In my quick test, I had something like this:

    fn append_compressed_dname<N: ToDname + ?Sized>(
        &mut self,
        name: &N,
    ) -> Result<(), Self::AppendError> {
        Composer::append_compressed_dname(*self, name)
    }

@xofyarg xofyarg force-pushed the add-composer-impls branch from b657496 to 96b4f4d Compare October 17, 2023 15:51
@partim
Copy link
Member

partim commented Oct 17, 2023

Sorry, to be annoying, but the same should be done for can_compress as well.

The use case is to share the underlying Composer buffer between
builders to avoid allocation, via a thread local variable for example.
Without the reference type implements, the ownership could be
transferred by memory swap operation, but that's not quite ideal. Plus
some methods like MessageBuilder::from_target would drop the Composer
on error, which makes it even difficult to share.
@xofyarg xofyarg force-pushed the add-composer-impls branch from 96b4f4d to 3701695 Compare October 17, 2023 16:40
@xofyarg
Copy link
Contributor Author

xofyarg commented Oct 17, 2023

My bad, I got up early this morning, with my brain left behind. Thought the can_compress could use the default implementation, but it's better to associate it with the type being referenced. Thanks for the gatekeeping.

@partim
Copy link
Member

partim commented Oct 18, 2023

Looking good now; thank you for the PR!

@partim partim merged commit 1b8a37e into NLnetLabs:main Oct 18, 2023
12 checks passed
partim added a commit that referenced this pull request Oct 18, 2023
Breaking changes

* Move the `flatten_into` method for converting domain names into a
  straight, flat form into a new `FlattenInto` trait. This trait is only
  implemented for types that actually are or contain domain names. ([#216])
* Marked various methods and functions that return values without side
  effects as `#[must_use]`. ([#228] by [@WhyNotHugo])
* Changed the signature of `FoundSrvs::merge` to use a non-mut `other`.
  ([#232])

New

* Added support for the ZONEMD record type. ([#229] by [@xofyarg])
* Re-exported the _octseq_ crate as `dep::octseq`. ([#230])
* Added a blanket impl for mut refs to `Composer`. ([#231] by [@xofyarg])
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.

2 participants