OleksandrKvl / sbepp

C++ implementation of the FIX Simple Binary Encoding
https://oleksandrkvl.github.io/sbepp/
MIT License
39 stars 4 forks source link

Add `size_bytes()` for message/group/data traits #43

Closed OleksandrKvl closed 3 months ago

OleksandrKvl commented 3 months ago

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:

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 commented 3 months ago

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

OleksandrKvl commented 3 months ago

Makes sense, will add it to this PR later.

OleksandrKvl commented 3 months ago

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

OleksandrKvl commented 3 months ago

Fixed, ready to be merged.

ujos commented 3 months ago

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 commented 3 months ago

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).