Skip to content

Conversation

@StephanTLavavej
Copy link
Member

Reported by a major customer.

Our policy is that we attempt to be /W4 clean but not /Wall clean, as /Wall contains a large number of extremely noisy warnings, many of which aren't suitable for using in production (some are intended for focused investigations, or were added for one unusual scenario, or were just way noisier than their value proved to be).

However, we generally do attempt to keep the codebase clean with respect to the off-by-default warning C4365, as we build our tests with /w14365:

* PM_CL="/FIforce_include.hpp /w14365 /w14668 /w15267 /D_ENFORCE_FACET_SPECIALIZATIONS=1"

Under the extremely obscure compiler option /J, char is treated as an unsigned type. This triggers C4365 signed/unsigned mismatch warnings when assigning promoted int to char in the vector<bool> optimization for copy() implemented by #3353 (merged 2023-09-21, shipped in MSVC Build Tools 14.39 / VS 2022 17.9).

Avoiding those warnings is easy, so even though /J is rarely used, and we don't officially support being C4365 clean, we may as well silence them and add test coverage.

Enabling the test coverage is significantly more obnoxious but all of the issues were in the test itself and are avoidable with repetitive casts.

…char` (when treated as unsigned under `/J`).
…on`.

Add lots of `static_cast<ptrdiff_t>` and a few `static_cast<size_t>`.

prefix.lst is what adds `/w14365`.

Cleanup: Avoid unnecessary `next()`.
@StephanTLavavej StephanTLavavej requested a review from a team as a code owner January 14, 2026 23:48
@StephanTLavavej StephanTLavavej added the enhancement Something can be improved label Jan 14, 2026
@github-project-automation github-project-automation bot moved this to Initial Review in STL Code Reviews Jan 14, 2026
@StephanTLavavej StephanTLavavej moved this from Initial Review to Final Review in STL Code Reviews Jan 15, 2026
Copy link
Member

@davidmrdavid davidmrdavid left a comment

Choose a reason for hiding this comment

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

LGTM, but will re-approve if/when this is applied: 655e813#r2695480146

@StephanTLavavej
Copy link
Member Author

@davidmrdavid I've pushed a commit to use unsigned char in these functions. Even if it doesn't help with the warning, I agree that it's conceptually cleaner to remain in the unsigned domain, and to avoid char squirrelliness. (I also changed the comments since it was weird to say plain "char", but left the _ch naming.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Something can be improved

Projects

Status: Final Review

Development

Successfully merging this pull request may close these issues.

4 participants