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

Fix C4365: '=': signed/unsigned mismatch with /J #3295

Merged
merged 2 commits into from Jan 11, 2023

Conversation

fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Dec 17, 2022

Fixes #3294

@fsb4000 fsb4000 requested a review from a team as a code owner December 17, 2022 05:17
@strega-nil-ms
Copy link
Contributor

I think this is a bug in the compiler; similar code without all of the stuff around it does not fire this warning. I would prefer we fix the compiler rather than work around it in the library, (or at least mark it with a transition).

@strega-nil-ms strega-nil-ms added this to Initial Review in Code Reviews via automation Dec 19, 2022
@strega-nil-ms
Copy link
Contributor

Okay, now that I look at it, it is an off-by-default warning; I personally think it's very silly to warn for:

unsigned char c = ...;
c = c + 1;

and I'd prefer we just turned off that warning or something along those lines.

@strega-nil-ms strega-nil-ms added the decision needed We need to choose something before working on this label Dec 19, 2022
@fsb4000
Copy link
Contributor Author

fsb4000 commented Dec 20, 2022

We already have the same static_cast for fixing the warning:

*_First++ = static_cast<char>('0' + __digits);

_First is char* and __digits is uint32_t, this is why it is triggered without /J

@strega-nil-ms
Copy link
Contributor

I disagree with that - __digits is not the same type as the type we're assigning into, which means it makes sense (to me) to cast it down - it's a narrowing cast.

The unsigned char thing is just a silly result of C's arithmetic promotion rules. It should not be treated as something to warn on.

@StephanTLavavej StephanTLavavej added bug Something isn't working and removed decision needed We need to choose something before working on this labels Jan 4, 2023
@StephanTLavavej
Copy link
Member

We talked about this at the weekly maintainer meeting; warning C4365 is indeed one of the few off-by-default warnings we attempt to be clean for:

PM_CL="/FIforce_include.hpp /w14365 /D_ENFORCE_FACET_SPECIALIZATIONS=1 /D_STL_CALL_ABORT_INSTEAD_OF_INVALID_PARAMETER"

We additionally (grudgingly) support /J, in the running for the worst compiler option ever, so this is technically a bug.

Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I severely dislike both this warning and this option, but it is a correct change. Thanks!

@CaseyCarter CaseyCarter moved this from Initial Review to Final Review in Code Reviews Jan 4, 2023
@CaseyCarter CaseyCarter moved this from Final Review to Ready To Merge in Code Reviews Jan 4, 2023
@StephanTLavavej
Copy link
Member

Thanks for the fix and updated test coverage! This is a minimally invasive change for the Ryu-derived code.

@StephanTLavavej StephanTLavavej self-assigned this Jan 11, 2023
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej
Copy link
Member

I had to push an additional commit to fix the internal test harness, as /clr:pure is incompatible with /J. I've chosen to change the usual_matrix to the impure_matrix, omitting a bit of unnecessary test coverage. (There are an extremely small number of /clr:pure users, virtually all of which are in C++14 mode, so the combination of /clr:pure and /Zc:alignedNew- isn't worth testing.)

@StephanTLavavej StephanTLavavej merged commit 78fd095 into microsoft:main Jan 11, 2023
Code Reviews automation moved this from Ready To Merge to Done Jan 11, 2023
@StephanTLavavej
Copy link
Member

Thanks for fixing this warning! 🛠️ ⚠️

@fsb4000 fsb4000 deleted the fix3294 branch January 12, 2023 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

<chrono>: building with -J emits C4365
4 participants