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

Use impl Trait return type for next_string_reader and string_value_writer #18

Closed
Marcono1234 opened this issue Oct 23, 2023 · 1 comment
Labels
enhancement New feature or request

Comments

@Marcono1234
Copy link
Owner

Problem solved by the enhancement

Currently JsonReader::next_string_reader and JsonWriter::string_value_writer have Box<...> as return type because:

  • They don't want to expose the internal implementation structs as public API
  • Custom JsonReader and JsonWriter implementation should be able to implement those methods

However, this usage of Box means that heap allocations are performed.

Enhancement description

Use the impl Trait in return positions feature (RFC 3425) once it is available in stable Rust.

This allows avoiding usage of Box while still addressing the concerns listed above for why Box is currently used.

A proof-of-concept implementation of this is in https://github.com/Marcono1234/struson/tree/impl-trait-return (has to be slightly adjusted once this Rust feature is part of the stable toolchain).

Alternatives / workarounds

  • Define public structs for string value reader and writer and declared them as return type;
    that would however prevent custom JsonReader and JsonWriter implementations from implementing the methods returning these value readers and writers
@Marcono1234 Marcono1234 added the enhancement New feature or request label Oct 23, 2023
@Marcono1234
Copy link
Owner Author

The Rust blog post discourages using -> impl Trait in public API. However, since the current API returns Box<dyn Read + '_> respectively Box<dyn StringValueWriter + '_>, using -> impl Trait seems equally good / bad, except that as mentioned above it avoids the heap allocations from Box.

So using -> impl Trait should probably be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

When branches are created from issues, their pull requests are automatically linked.

1 participant