Patch: bug fix for PR18218

Marshall Clow mclow.lists at gmail.com
Fri Jan 3 08:27:25 PST 2014


On Dec 29, 2013, at 3:24 PM, Howard Hinnant <howard.hinnant at gmail.com> wrote:

> On Dec 29, 2013, at 11:53 AM, Marshall Clow <mclow.lists at gmail.com> wrote:
> 
>> Here’s a patch for review.
>> It only deals with “pow”, but the same techniques work on all the other calls.
>> 
>> [ I chose “pow” because there’s a “pow” in <complex>, and we have to not step on that. ]
>> 
>> Anyway - if we have rvalue references, then we’ll use them. If not, then we pass by value.
>> We can’t pass by const &, because the conversion operator might not be const.
>> We can’t pass by non-const &, because that would fail on literal values.
>> 
>> The exception specification depends on conversion to floating point.
>> Note that pow (double, double) is NOT noexcept - because that’s defined elsewhere :-(, and we can’t change that.
>> [ Even though it will never throw, we can’t mark it as noexcept ]
>> 
>> noexcept is QOI - it’s not specified in the standard.
>> 
> 
> This is looking good.  Thanks much for all of the work you've put into this!
> 
> A counter patch is enclosed below, closely related to the patch you have.
> 
> Notable differences:
> 
> 1.  __numeric_type has been demoted to an implementation detail of __promote.  The motivation for this is to simply the code in <cmath> as much as possible since it has to be repeated for each math function.
> 
> 2.  __promote<A1, A2, ...> now has a nested "type" that may be computed from a user-defined type X with an implicit, non-ambiguous conversion to an arithmetic type (just like __numeric_type).
> 
> 3.  __promote<A1, A2, ...>::type now does not exist if the conversion for each A does not exist (like __numeric_type, but expanded to multiple arguments now).  This enables __promote to act as its own enable_if, simplifying the return type of most of the math functions.
> 
> 4.  If __promote<A1, A2, ...> also has a nested bool: value.  This is true if the nested type exists, else it is false.  This is handy for enable_if'ing isnan, which returns bool instead of the "generic double".
> 
> 5.  __promote<A1> (single argument only) will contain another const static bool:  __does_not_throw, which is true if the conversion from A1 to __promote<A1>::type does not throw an exception.  This simplifies the noexcept spec out at the math function level.
> 
> 6.  Added perfect forwarding to the return statement in pow.  This makes sure things work if the conversion operator has an "rvalue-this" qualification.
> 
> 7.  Added remove_reference to the static_assert within pow.  This static_assert is designed to prevent infinite recursion due to a logic bug in this design.  It basically says:  Don't call this templated thing again!
> 
> 8.  Because isnan is a little different, I've prototyped that too.  Now isnan has ordinary float, double and long double overloads (like pow, but in the global namespace).  And the templated isnan is moved inside of namespace std.  Because it always returns bool, it has an explicit enable_if based on __promote<_A1>::value (does the conversion exist), instead of on whether or not the nested "type" exists in __promote.
> 
> Tests seem to be passing.  But this needs careful review from all interested parties.

This LGTM.
I think it addresses all the issues that we’ve seen.

Sorry it took me so long to respond.

— Marshall

P.S.	It doesn’t actually solve all of PR18218; but it gives us the tools needed to solve the rest of the bug.





More information about the cfe-commits mailing list