No, sorry, this pragma party went too far. Why not removing day < 0 with the the 5-lines long comment?

If you want to validate that day type is signed, there are ways to check it using Py_BUILD_ASSERT() and something like:

#define IS_TYPE_UNSIGNED(type) (((type)0 - 1) > 0)

Thanks for the review, I will update it after @pganssle read this comment :)

This is a bit excessive. We solved the problem differently in the backport, though I guess I didn't know that the warning to disable is different on clang, or the extent of the gcc support. TBH I feel like maybe we shouldn't care this much about persnickety and frankly unnecessary compiler warnings.

I like Victor's idea of a compile-time assertion — it was my original hope, but the problem is that we don't want to do Py_BUILD_ASSERT(IS_TYPE_UNSIGNED(uint8_t)), that's a trivial assertion and not worth doing. We need to assert that the variable day is an unsigned type, which means we'd need to get its type at compile time, then pass it through these various macros (as an aside, Py_BUILD_ASSERT gives remarkably unhelpful error messages). I haven't found any generic cross-platform way to do that in what googling I've been able to do, but maybe Victor has a solution here.

I'm inclined to do our best effort at disabling compiler warnings, but at the end of the day, this is the problem of an overzealous compiler, not a problem with the code. The code is deliberately describing the actual bounds of the function, it's not an oversight where we didn't realize it's an unsigned type or something. I don't think we need to let the blunt dictates of the compiler tell us how to code.

Honestly, I don't see the purpose of the whole thing, and I would suggest to simply remove it.

If you want to make extra careful, you can use something like:

Py_BUILD_ASSERT(IS_TYPE_UNSIGNED(tpyeof(day)))

But I never see something like that in the CPython code base. If the code changes tomorrow, we do have reviews to detect bugs. And if a bug happens, well, we can fix it.

But preventing bugs with 10 lines or 20 lines of bugs, whereas the current code is fine, sounds overkill to me.

You can keep /* day < 0 || /* day > 6 as a comment if you want to be more very explicit.

Honestly, I don't see the purpose of the whole thing, and I would suggest to simply remove it.

What you're proposing is that we make this code more fragile to satisfy overzealous compiler warnings. Why? The only downside to keeping the compiler at zero warnings is that it makes it easier for us to enforce that we're at zero warnings, and based on the quality of some of these warnings, I'm not sure we should care about that.

Py_BUILD_ASSERT(IS_TYPE_UNSIGNED(typeof(day)))

I personally couldn't get this to work for some reason, but even so, I think typeof is a GCC extension, I don't think it's portable. I think we might need some complicated compiler-specific logic to do static assertions about the type of a variable.

But I never see something like that in the CPython code base. If the code changes tomorrow, we do have reviews to detect bugs. And if a bug happens, well, we can fix it.

I suspect the reason that this is rarely done in CPython is that it's usually easier to just write the code in the way that the compiler won't complain about, even if it's worse than the way they want to write it because it's such a pain to disable the warnings.

If you think it's really critical that we never see any warnings on any compilers, then maybe it makes sense to write a more generic macro for suppressing warnings.

The only downside to keeping the compiler at zero warnings is that it makes it easier for us to enforce that we're at zero warnings, and based on the quality of some of these warnings, I'm not sure we should care about that.

That's what we are trying to do here: #18532. And we will cause every PR to show those warnings if we don't remove it. In general, these warnings show potential bugs and that's why we want to start showing them in PRs. Many people commit changes without any warnings on Linux that show potential problems in Windows that they never see because the warnings in the build log do not appear in the PR.

I don't think we need to let the blunt dictates of the compiler tell us how to code.

Don't say that out loud or rustc may hear it ;)

That's what we are trying to do here: #18532. And we will cause every PR to show those warnings if we don't remove it. In general, these warnings show potential bugs and that's why we want to start showing them in PRs.

If we're going to start enforcing no compiler warnings on all compilers in all cases, then we need an easy way to disable them. Removing day < 0 here would make this code worse, which is why I didn't do it in the first place.

Maybe it's not a huge deal in this case, but it's a bad precedent to set, it's like blindly trusting pylint and never disabling any of its nonsense lints.

I concur with @vstinner. Compiler warning is a red flag and it spends time of developers to check what bug or suspicious code there is. We should either fix the code if there is actual bug or silence warning by rewriting the code in different way or adding explicit pragmas. In this case the cure is worse than potential problem, because compiler-specific pragmas cases other compiler warnings, and testing the compiler and make the code too verbose. The simplest way is to remove the trivial check. We know that unsigned values are never negative.

I still consider that these complex compiler warnings are not worth it. This PR adds even more compiler pragmas.

I wrote a simple fix, with a short comment to warn future developers against changing the 'day' parameter type: PR #23614. My PR removes lines rather than adding lines. I wrote a longer rationale in comments of my PR.