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: code duplication part 2. #844

Closed
jzmaddock opened this issue Jan 29, 2025 · 1 comment · Fixed by #863
Closed

Boost Review: code duplication part 2. #844

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

Comments

@jzmaddock
Copy link

Code duplication: Importantly, I don't understand why the trait bias_v is even needed, given that it duplicates the value stored in std::numeric_limits<...>::digits/digits10/maxdigits10. Same story with the emin_v and emax_v traits compared to the std::numeric_limits<...>::min_exponent/min_exponent10 and std::numeric_limits<...>::max_exponent/max_exponent10. This seems like some code duplication that would be prone to causing confusion later. Why do multiple definitions of traits exist that have the same values? There may be other cases of this as well. I would love to see either std::numeric_limits versions being defined in terms of the other traits where possible, or vice-versa. I find it confusing to have 2 bespoke definitions.

@mborland mborland added documentation Improvements or additions to documentation Boost Review Collected Comments from Boost Review Period and removed documentation Improvements or additions to documentation labels Jan 29, 2025
@mborland
Copy link
Member

I think this will be closed as well by #863. We can't replace the traits with std::numeric_limits since the traits are used in the implementation of the classes. The traits are used to define our values in std::numeric_limits so there is consistency there.

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