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 size_bytes() for message/group/data traits #43

Merged
merged 7 commits into from
Apr 9, 2024

Conversation

OleksandrKvl
Copy link
Owner

@OleksandrKvl OleksandrKvl commented Apr 4, 2024

Implements functionality discussed in #33.

Here's a brief overview of considered design options.

sbepp::predict_size_bytes<T>(args...) where T is a representation type (e.g. schema_name::messages::msg_name). While it was the initial choice, further investigation discovered multiple issues with this approach:

  • being a generic function, it has to be a variadic template so can't expose required parameter names to the user
  • representation types work with binary data, while this function is not related to binary data in any way
  • representation types handle both encoding/decoding via the same interface, this function only makes sense for encoding
  • even if easily possible, it's still questionable that this can be a static function of representation type because it's not related to its functionality, it doesn't need anything from it, nor it provides anything to it.

Since it's static in nature, potential implementation of sbepp::predict_size_bytes<T> would require an internal struct with static function and its specializations for each representation type. At this point this looks much more like a trait so the following approach was chosen.

message_traits<Tag>::size_bytes(args...). Traits are already used to provide static/meta information, for example composite_traits::size_bytes() so after a bit of thinking, this looked like a natural way to go. While it's unlikely that someone will ever need group_traits::size_bytes() or data_traits::size_bytes(), they are used internally for message_size::size_bytes() so I see no reason to keep them private. Unlike sbepp::predict_size_bytes(), this approach allows user to see all the parameters which is definitely a benefit since they depend on the message structure. While using trait might be a bit verbose, #41 should make it possible to get trait tag from representation type to avoid using both types in code.

@ujos
Copy link

ujos commented Apr 4, 2024

Eventually would be great to add an example to the documentation of how this function is used in the message sending flow.

@OleksandrKvl
Copy link
Owner Author

Makes sense, will add it to this PR later.

@OleksandrKvl OleksandrKvl marked this pull request as draft April 4, 2024 16:51
@OleksandrKvl
Copy link
Owner Author

Added more tests and found a bug, making it a draft for now.

@OleksandrKvl OleksandrKvl marked this pull request as ready for review April 8, 2024 08:47
@OleksandrKvl
Copy link
Owner Author

Fixed, ready to be merged.

@ujos
Copy link

ujos commented Apr 8, 2024

Perfect, it works like a charm. The only minor issue is that the code of the size_bytes() function is not aligned properly where it comes to the varLength arrays and group entries.

@OleksandrKvl
Copy link
Owner Author

Improved parameter list formatting, now it's a parameter per line, not a single giant line. Don't see much reason to care about function body formatting because users are not supposed to read it (when I need to do this, I just open a header and apply clang-format to it).

@OleksandrKvl OleksandrKvl merged commit 8a4b1ef into main Apr 9, 2024
1 check passed
@OleksandrKvl OleksandrKvl deleted the OleksandrKvl/predict-size-bytes-impl branch April 9, 2024 13:09
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