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

visit_any_int incorrect signed long long casting #265

Closed
rglarix opened this issue Jan 26, 2016 · 4 comments
Closed

visit_any_int incorrect signed long long casting #265

rglarix opened this issue Jan 26, 2016 · 4 comments

Comments

@rglarix
Copy link

@rglarix rglarix commented Jan 26, 2016

Hi, I believe visit_any_int has an incorrect casting for the signed long long case.
Both branches of the if at line 329 of format.cc end up doing
arg_.long_long_value = static_cast < typename fmt::internal::MakeUnsigned < U>::Type>(value);

Perhaps the is_signed branch should do
arg_.long_long_value = static_cast < long long>(static_cast < T>(value));
instead ?

@vitaut

This comment has been minimized.

Copy link
Contributor

@vitaut vitaut commented Jan 26, 2016

Thanks for reporting this, but the existing code looks correct to me. The branches differ in that the first assigns to long_long_value and the second to ulong_long_value. The cast to unsigned is used for consistency with glibc's printf as in:

std::printf("%lld", -42);  // prints "4294967254" in glibc

although it's not technically necessary because this is UB.

I've added a comment to clarify this in ae6368c.

@vitaut vitaut closed this Jan 26, 2016
@rglarix

This comment has been minimized.

Copy link
Author

@rglarix rglarix commented Jan 26, 2016

First, many thanks for cppformat and your super-fast answer. Thank you, really.

Sorry to be a nuisance on a minor matter like this, but isn't this another instance of cppformat having better type information than C ?
As you said, this is undefined behavior, but cppformat can easily give a better answer by casting to the correct type, instead of unsigned.
Besides, while the current result is compatible with glibc, is different from Visual C++ 2013, which prints -4540733497999884330 (garbage) instead of -42.

vitaut added a commit that referenced this issue Jan 27, 2016
@vitaut

This comment has been minimized.

Copy link
Contributor

@vitaut vitaut commented Jan 27, 2016

Fair enough. I've removed the conversion to an unsigned type in 7ee287d, so fmt::printf should now do the Right Thing, namely sign extend the argument of a smaller signed type:

  fmt::printf("%lld", -42); // prints -42

Thanks for reporting this and for the nice feedback.

@rglarix

This comment has been minimized.

Copy link
Author

@rglarix rglarix commented Jan 27, 2016

thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.