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

<cmath>: A compile-time warning C4244: 'argument': conversion from '_Common' to 'double' #3246

Closed
steve02081504 opened this issue Nov 29, 2022 · 4 comments · Fixed by #3253
Closed
Labels
bug Something isn't working fixed Something works now, yay!

Comments

@steve02081504
Copy link

Describe the bug
I found this annoying warning when I was idly testing my library and I'd like it to be dealt with reasonably well if possible

Command-line test case

#include <type_traits>
#include <cmath>
template<typename T> requires ::std::is_arithmetic_v<T>
constexpr T copy_as_negative(auto x,bool negative=1)noexcept{
	if constexpr(::std::is_signed_v<decltype(x)>){
		if constexpr(::std::is_floating_point_v<decltype(x)>)
			return(T)::std::copysign(x,negative?-1:1);
		else
			return(T)negative?T{}-x:x;
	}
	else
		return x;
}
constexpr auto copy_as_negative(auto x,bool negative=1)noexcept{
	return copy_as_negative<decltype(x)>(x,negative);
}
int main() {
	copy_as_negative(long double{0}, 1);
}

Compile this program together with /W4 and you will reasonably get a C4244 warning

Expected behavior
no C4244 warning plz

STL version

Microsoft Visual Studio Community 2022 (64 位) - Preview
17.5.0 Preview 1.0

Additional context
The original code is at https://godbolt.org/z/3eav315a1 if anyone needs it

@frederick-vs-ja
Copy link
Contributor

long double{0} is ill-formed, despite recognized by MSVC as an extension. You should use 0.0l instead.

I guess this is a compiler bug - double and long double have the same format on MSVC so it's wrong for MSVC to say "conversion from '_Common' to 'double', possible loss of data".

@steve02081504
Copy link
Author

long double{0} is ill-formed, despite recognized by MSVC as an extension. You should use 0.0l instead.

I guess this is a compiler bug - double and long double have the same format on MSVC so it's wrong for MSVC to say "conversion from '_Common' to 'double', possible loss of data".

Thank you for your help, I have submitted two issues on this in the vs feedback section
https://developercommunity.visualstudio.com/t/warning-C4244:-argument:-conversion-fr/10215707
https://developercommunity.visualstudio.com/t/Optimisation-makes-code-crash/10215710
Should I close this issue now?

@CaseyCarter CaseyCarter added the bug Something isn't working label Nov 30, 2022
@CaseyCarter
Copy link
Member

CaseyCarter commented Nov 30, 2022

I agree with @frederick-vs-ja that the compiler shouldn't emit a warning here, but I also think that we should workaround the warning for this case as we do for many other _HAS_CMATH_INTRINSICS cases - such as copysign(long double, long double):

STL/stl/inc/cmath

Lines 334 to 343 in e28f956

_NODISCARD _Check_return_ inline long double copysign(_In_ long double _Number, _In_ long double _Sign) noexcept
/* strengthened */ {
#if _HAS_CMATH_INTRINSICS
return __copysign(static_cast<double>(_Number), static_cast<double>(_Sign));
#elif defined(__clang__)
return __builtin_copysignl(_Number, _Sign);
#else // ^^^ defined(__clang__) ^^^ / vvv intrinsics unavailable vvv
return _CSTD copysignl(_Number, _Sign);
#endif // ^^^ intrinsics unavailable ^^^
}

- by converting the long double arguments to double when passing them to the intrinsic.

@CaseyCarter
Copy link
Member

CaseyCarter commented Nov 30, 2022

Minimized:

// Compile with "cl /nologo /W4 /WX /Zs /diagnostics:caret"
extern "C" double __copysign(double, double);

void f(long double x, long double y) {
    (void) __copysign(x, y); // C4244 (two places)
}

Specifically, the compiler is diagnosing the conversions of x and y to double in the call to __copysign.

CaseyCarter added a commit to CaseyCarter/STL that referenced this issue Dec 1, 2022
This template implements the "sufficient additional overloads" for two-argument `<cmath>` functions (`atan2`, `hypot`, `pow`, `fmod`, `remainder`, `copysign`, `nextafter`, `fdim`, `fmax`, and `fmin`) by calling either an intrinsic or a C library function either of which expects `double` arguments. We previously converted the arguments to `_Common_float_type_t` which is `long double` when at least one argument is `long double`, resulting in narrowing conversion warnings at `/W4`. We now convert the arguments to `double` instead of `_Common_float_type_t`.

Fixes microsoft#3246
CaseyCarter added a commit to CaseyCarter/STL that referenced this issue Dec 1, 2022
This template implements the "sufficient additional overloads" for two-argument `<cmath>` functions (`atan2`, `hypot`, `pow`, `fmod`, `remainder`, `copysign`, `nextafter`, `fdim`, `fmax`, and `fmin`) by calling either an intrinsic or a C library function either of which expects `double` arguments. We previously converted the arguments to `_Common_float_type_t` which is `long double` when at least one argument is `long double`, resulting in narrowing conversion warnings at `/W4`. We now convert the arguments to `double` instead of `_Common_float_type_t`.

Fixes microsoft#3246
@StephanTLavavej StephanTLavavej added the fixed Something works now, yay! label Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed Something works now, yay!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants