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

Boost Review: Rationalize traits class usage to make the code easier to read. #842

Closed
jzmaddock opened this issue Jan 29, 2025 · 0 comments · Fixed by #863
Closed

Boost Review: Rationalize traits class usage to make the code easier to read. #842

jzmaddock opened this issue Jan 29, 2025 · 0 comments · Fixed by #863
Labels
Boost Review Collected Comments from Boost Review Period

Comments

@jzmaddock
Copy link

(1) Sometimes there are traits used, such as detail::bias_v<>, but other times the trait's functionality is replicated in-line. I find this more difficult to read, instead of using the trait. As an example, in the function detail::to_chars_impl(), we have this line:

constexpr auto max_fractional_value = std::is_same<TargetDecimalType, decimal32>::value || std::is_same<TargetDecimalType, decimal32_fast>::value ? TargetDecimalType{1, 7} : std::is_same<TargetDecimalType, decimal64>::value || std::is_same<TargetDecimalType, decimal64_fast>::value ? TargetDecimalType{1, 16} : TargetDecimalType{1, 34};

This line could easily be replaced with the following, which means exactly the same thing, but is easier to visually parse.

constexpr auto max_fractional_value = TargetDecimalType{1, bias_v<TargetDecimalType>};

I don't want this to sound like a nit-pick, but cases like this have made it more difficult for me to understand and work through the code.

(2) On the topic of traits, I think there should be some more traits defined, to remove usage of things like std::is_same<...>::value. This is another thing that has made it more difficult for me to read the code. Something along the lines of detail::is_fast<...> to denote whether you have a _fast type or not. For example, you could replace the following

std::enable_if_t<std::is_same<DecimalType, decimal32>::value || std::is_same<DecimalType, decimal64>::value || std::is_same<DecimalType, decimal128>::value, bool>

With the following

std::enable_if_t<!detail::is_fast<DecimalType>::value, bool>

As well, detail::bit_width<...> would help remove lines like std::is_same<T, decimal128>::value || std::is_same<T, decimal128_fast>::value, and turn them into detail::bit_width<...>::value == 128. These are just suggestions and don't need to be taken verbatim. Anything to remove duplication and make the code easier to understand.

@mborland mborland added the Boost Review Collected Comments from Boost Review Period label Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Boost Review Collected Comments from Boost Review Period
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants