Clean-up sign-conversion warnings (rebased) #1440
Conversation
0x8000-0000
changed the title
0x8000-0000
changed the title
Welcome to GitHub - you might want to start with https://guides.github.com/activities/hello-world/ |
include/fmt/chrono.h
Outdated
| : context(ctx), out(o), val(d.count()), negative(false) { | ||
| if (d.count() < 0) { | ||
| val = 0 - val; | ||
| : context(ctx), out(o), val(static_cast<rep>(std::abs(d.count()))), negative(d.count() < 0) { | ||
|
|
||
| if (d.count() < 0) { | ||
| val = static_cast<rep>(- d.count()); | ||
| negative = true; | ||
| } | ||
| } | ||
| else { | ||
| val = static_cast<rep>(d.count()); | ||
| negative = false; | ||
| } |
There was a problem hiding this comment.
There is too much redundancy here. Please keep the old code and just add a cast.
There was a problem hiding this comment.
I apologize for the line 796, it was mismerged by me. I intended to remove the member initialization of val and negative.
But we can't keep the old code, since it assigns a possibly signed value "d.count()" into an unsigned variable "val". You need "val" to acquire the absolute value of d.count, which can be cleanly done either with "std::abs()" like on line 796, or with the test on 798:804.
Which one would you prefer?
There was a problem hiding this comment.
I think it should be enough to add static_cast to the initializer list.
I will update as requested; this means I have to keep maintaining my fork that builds with -Wsign-conversion for a while, presumably until we get C++20 conformant compilers. |
0x8000-0000
force-pushed
the
no-no-sign-conversion-reviewed
branch
from
476b917
to
996a699
Compare
|
Merged with minor tweaks, thanks. |
I agree that my contributions are licensed under the {fmt} license, and agree to future changes to the licensing.
This pull request contains the reviewed and agreed-upon portion of #1435, rebased on top of current master.