Skip to content

bpo-9566 & bpo-30747: Silence warnings from pyatomic.h macros#3140

Merged
pitrou merged 3 commits into
python:masterfrom
segevfiner:bpo-9566-pyatomic-warnings
Aug 20, 2017
Merged

bpo-9566 & bpo-30747: Silence warnings from pyatomic.h macros#3140
pitrou merged 3 commits into
python:masterfrom
segevfiner:bpo-9566-pyatomic-warnings

Conversation

@segevfiner

@segevfiner segevfiner commented Aug 18, 2017

Copy link
Copy Markdown
Contributor

Apparently MSVC is too stupid to understand that the alternate branch is not taken and emits a warning for it:

warning C4133: 'function': incompatible types - from 'volatile uintptr_t *' to 'volatile int *'

Warnings added in #2383.

cc @Paxxi @pitrou

https://bugs.python.org/issue9566

Apparently MSVC is too stupid to understand that the alternate branch is
not taken and emits a warning for it.

Warnings added in python#2383
@pitrou

pitrou commented Aug 18, 2017

Copy link
Copy Markdown
Member

Instead of adding obscure pragmas, wouldn't be it more readable to add the necessary casts?
(though of course the end result -- suppressing the warnings -- would be the same)

@segevfiner

Copy link
Copy Markdown
Contributor Author

Instead of adding obscure pragmas, wouldn't be it more readable to add the necessary casts?
(though of course the end result -- suppressing the warnings -- would be the same)

How can I cast to ignore the warning? The problem, to my understanding, is that MSVC thinks that the other branch of the if or ternary can be taken for some reason. So, for example, if we pass a volatile uintptr_t*, it thinks _Py_atomic_load_32bit can be called, so it emits the warning. But maybe I'm wrong...

@pitrou

pitrou commented Aug 18, 2017

Copy link
Copy Markdown
Member

Simply cast the relevant parameters to _Py_atomic_load_32bit? Perhaps I'm missing something.

@segevfiner

Copy link
Copy Markdown
Contributor Author

@pitrou Done. Used volatile long long* and volatile long* since that's what the Interlocked* functions take.

@Paxxi

Paxxi commented Aug 18, 2017

Copy link
Copy Markdown
Contributor

I was just writing about that solution when I saw the commits being pushed :)

I think this is much saner, possibly use uintptr_t instead of long long just to keep it consistent?

Thank you for looking into this!

@segevfiner

Copy link
Copy Markdown
Contributor Author

uintptr_t is 32-bit in a 32-bit build which you will than pass to _Py_atomic_load_64bit...

@Paxxi

Paxxi commented Aug 18, 2017

Copy link
Copy Markdown
Contributor

there's another macro for 32bit which doesn't contain the ternary operator as there's no 64bit operations required there.

@segevfiner

segevfiner commented Aug 18, 2017

Copy link
Copy Markdown
Contributor Author

there's another macro for 32bit which doesn't contain the ternary operator as there's no 64bit operations required there.

I think the _Py_atomic_store_explicit macro I modified is used for both 32-bit and 64-bit MSVC builds (Include/pyatomic.h:234). _Py_atomic_store_64bit is defined to just return the value directly on 32-bit builds (Include/pyatomic.h:266).

There is another similar _Py_atomic_store_explicit below but I think that one is for Windows on ARM...

@Paxxi

Paxxi commented Aug 18, 2017

Copy link
Copy Markdown
Contributor

seems I remembered it wrong. You're entirely correct.

@pitrou pitrou merged commit 0267128 into python:master Aug 20, 2017
@segevfiner segevfiner deleted the bpo-9566-pyatomic-warnings branch August 21, 2017 10:27
GadgetSteve pushed a commit to GadgetSteve/cpython that referenced this pull request Sep 10, 2017
…#3140)

* bpo-9566: Silence warnings from pyatomic.h macros

Apparently MSVC is too stupid to understand that the alternate branch is
not taken and emits a warning for it.

Warnings added in python#2383

* bpo-9566: A better fix for the pyatomic.h warning

* bpo-9566: Remove a slash
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants