-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Appease MSVC Warning C4866: compiler may not enforce left-to-right evaluation order #5953
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
Conversation
Skylion007
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird warning, but makes sense.
The previous comment referenced the MSVC warning but didn't explain why the code is structured as two statements. The revised comment clarifies the intent: fetching the value first ensures well-defined evaluation order.
Switch from crate-ci/typos to adhtruong/mirrors-typos because pre-commit autoupdate confuses tags in the upstream repo, selecting the mutable `v1` tag instead of pinned versions like `v1.41.0`. See crate-ci/typos#390
rwgk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a couple trivial commits.
Will merge after I see that the CI has finished.
|
Hm, I don't remember seeing this failure (EDIT: it was resolved by a rerun): CI / 🐍 (macos-latest, 3.14t, -DCMAKE_CXX_STANDARD=20) / 🧪 (pull_request) |
| // Fetch value before indexing into kwargs to ensure well-defined | ||
| // evaluation order (MSVC C4866). | ||
| PyObject *const arg_in_arr = args_in_arr[n_args_in + i]; | ||
| kwargs[PyTuple_GET_ITEM(kwnames_in, i)] = arg_in_arr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The warning still exists. The problem may be that the order of key casting and assignment is not well-defined.
C:\Users\runneradmin\AppData\Local\Temp\pip-build-env-tsdbyp__\overlay\Lib\site-packages\pybind11\include\pybind11\pybind11.h(1087,1): error C2220: the following warning is treated as an error [C:\Users\runneradmin\AppData\Local\Temp\tmph2yqzrcl.build-temp\Release\src\_C.vcxproj]
C:\Users\runneradmin\AppData\Local\Temp\pip-build-env-tsdbyp__\overlay\Lib\site-packages\pybind11\include\pybind11\pybind11.h(1087,1): warning C4866: compiler may not enforce left-to-right evaluation order for call to 'pybind11::detail::object_api<pybind11::handle>::operator[]' [C:\Users\runneradmin\AppData\Local\Temp\tmph2yqzrcl.build-temp\Release\src\_C.vcxproj]|
Do you have a way to test locally first? |
Yes, working on it. |
Description
This is a regression after #5948
Piggy-backed commit f16de66:
Switch
typospre-commit hook to a mirror repo (adhtruong/mirrors-typos) to fixpre-commit autoupdateselecting the mutablev1tag instead of pinned versions. See crate-ci/typos#390Suggested changelog entry: