-
Notifications
You must be signed in to change notification settings - Fork 221
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
constexpr frexp #686
constexpr frexp #686
Conversation
Matt, thanks for pushing ahead on this function and the My tests only used my implementation within the context of larger I am, however, convinced that the addess operator of the signed integer exponent argument (i.e., as in I will need to look at your implementation @mborland more closely. Once a differentiation is made between C++11/14-and-beyond
Unlike the traditional code above, however, you could use the |
@ckormanyos This code is all written using C++17 so the definition of
I am not entirely sure this error can be avoided. |
You can't use the function in that manner: it would have to be within the body of a larger constexpr function. |
Pending review this is clean on CI. Next in the queue is |
doc/sf/ccmath.qbk
Outdated
template <typename Real> | ||
inline constexpr Real frexp(Real arg, int* exp); | ||
|
||
template <typename Z> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trivial quibble, but I when I first read this I didn't grok that this was limited to integer arguments, maybe use <typename Integer>
and perhaps a comment that this is for integer arguments?
Other than that, I'm happy for this to be merged, and you'll be pleased to know that ldexp should be noticeably easier! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jzmaddock : That was my (perhaps poor) convention; mimicking ℤ.
Is it possible to do a concept without breaking C++17 compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, although the enable_if does much the same thing. It's debatable whether the enable_if condition should be part of the declared interface - it clutters up the docs, but does at least make it clear what the requirements on Z are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc suggestions have been incorporated in the latest commit.
As for concepts, you can avoid breaking C++17 compatibility using this macro (taken from good concepts):
#ifdef GOOD_COMPILER
#define REQUIRES requires
#elseif
#define REQUIRES //
#endif
@ckormanyos This was borrowed heavily from your wide integer implementation with some added handling described in the standard (e.g. NaN). Do you have a sample
constexpr
test you use? I am not sure how to get aroundexp
having external visibility in aconstexpr
context. I could change to something likefrexp(T arg, void)
, but then the behavior would be different from the standard.