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

Redeclaration of max_factorial value #784

Merged
merged 1 commit into from
May 10, 2022

Conversation

ckormanyos
Copy link
Member

@ckormanyos ckormanyos commented May 10, 2022

I found this one while snooping around with older compiler(s) and other projects. But it also influences Math/Multiprecision directly.

This minute change raises the question of exactly how should we deal with static constexpr POD members of a class/struct? Even in the presence of BOOST_NO_INCLASS_MEMBER_INITIALIZATION do these really need out-of-class initialization for any of the compilers we are using?

That there is a mismatch between const and constexpr is truly wrong. But what is right or less wrong?

I addressed this question in my own developments about two years ago while modernizing and came to the conclusion that the extra out-of-class initialization is redundant, harmless and somehow is handled properly (or simply disregarded) by every old, new, embedded, etc. compiler that I use from 11, 14, 17, to 20 and beyond. So I synchronized the word constexpr and retained all such initializations in my projects.

So within the context of this minute change in this PR, maybe some dialog regarding our philosophy in Boost will ensue?

Cc: @jzmaddock and @mborland and @NAThompson.

@ckormanyos
Copy link
Member Author

This few-letter change is ready for review/discussion

@jzmaddock
Copy link
Collaborator

Looks fine to me.

@ckormanyos ckormanyos merged commit 4d5cc69 into develop May 10, 2022
@ckormanyos
Copy link
Member Author

Looks fine to me.

Thank you , John!

See also Multiprecison 429

@ckormanyos ckormanyos deleted the redeclaration_of_max_factorial branch May 10, 2022 17:47
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