GH-131296: fix clang-cl warning on Windows in sre.c - #131593
Conversation
Sorry, something went wrong.
| #if defined(_MSC_VER) | ||
| #pragma optimize("agtw", on) /* doesn't seem to make much difference... */ | ||
| # if defined(__clang__) | ||
| # pragma optimize("", on) |
There was a problem hiding this comment.
Fix warning : unexpected argument 'agtw' to '#pragma optimize'; expected "" [-Wignored-pragmas]
Sorry, something went wrong.
| # if defined(__clang__) | ||
| # pragma optimize("", on) | ||
| # else | ||
| # pragma optimize("gt", on) /* doesn't seem to make much difference... */ |
There was a problem hiding this comment.
agtw has been used in PY_LOCAL_AGGRESSIVE until #87332. Quote from @gvanrossum #87332 (comment)
A coworker happened to look at our use of #pragma optimize() for Windows (VS 2017) and noticed:
unless there’s something I’m missing, the ‘a’ and ‘w’ portions of the string being passed to the optimize pragma are not doing anything; I’m pretty sure they’ve been useless for 10+ years. The ‘g’ means turn on global optimizations and the ‘t’ means optimize for “time” (i.e. speed). See the documentation at https://docs.microsoft.com/en-us/cpp/preprocessor/optimize?view=msvc-160
It was then changed to gt (https://github.com/python/cpython/pull/24485/files) and entirely removed in https://github.com/python/cpython/pull/32023/files.
Sorry, something went wrong.
There was a problem hiding this comment.
@zooba Again a very Windows (MSVC) specific one. Back then, you've changed PY_LOCAL_AGGRESSIVE from agtw to gt - agtw is really dusty.
OTOH, even though the comment says /* doesn't seem to make much difference... */ - I think it does, because this is also done for debug builds. But since /Ob1 is now used for debug builds (AFAIR by Victor), maybe this is not needed anymore at all?
Sorry, something went wrong.
|
I expect @mdboom will have looked more closely at this one. Chances are we just want a !__clang__ condition and to do nothing at all in ClangCL. |
Sorry, something went wrong.
|
Sure, that's the easiest thing to do in case we do not want to care about
|
Sorry, something went wrong.
Yeah, I agree. |
Sorry, something went wrong.
|
Ups - I wanted to update the PR, but the PR is closed - do you want me to create a new PR instead of addressing the suggested fix: |
Sorry, something went wrong.
No, sorry. Just human error (I pressed the wrong button). |
Sorry, something went wrong.
| # else | ||
| # pragma optimize("gt", on) /* doesn't seem to make much difference... */ | ||
| # endif | ||
| #if defined(_MSC_VER) && !defined(__clang__) |
There was a problem hiding this comment.
Much simpler - just ignore __clang__ and keep agtw for MSVC.
Sorry, something went wrong.
I think this is a skip news?