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
Avoid const-ness compiler warning in __Pyx_PyObject_AsWritableString #2775
Conversation
Cython/Utility/ModuleSetupCode.c
Outdated
typedef unsigned __int32 uint32_t; | ||
#endif | ||
#endif | ||
#ifndef _UINTPTR_T_DEFINED | ||
#define _UINTPTR_T_DEFINED |
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 couldn't find this as a standard define anywhere, so I guess it's a name that you made up. In that case, we should prefix it with __PYX_
, which is Cython's internal standard prefix.
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.
(Although I wonder if this is needed at all – this code section can only appear once in each generated C file.)
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.
_UINTPTR_T_DEFINED is what my Visual Studio 2017 Community uses as guard around its uintptr_t typedef. So I think we should at least keep the #ifndef _UINTPTR_T_DEFINED. But whether we should #define _UINTPTR_T_DEFINED, I'm not so sure. It could avoid compilation issues, if the system header is included later on (by user code).
What do you think?
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.
If I read this page correctly, then MSVC defines uintptr_t
in stddef.h
. Maybe we should include that? (And then use the same guard as whatever they use?)
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.
BTW, if MSVC defines uintptr_t
in stddef.h
, then I think we should have a test that includes stddef.h
and exercises the AsWritableString
macro code, to make sure that both don't interfere.
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.
On my version of Visual Studio 2017 Community (which should be the latest one) uintptr_t is defined in vadefs.h.
I did not find another typedef in any other header file.
It's very similar to this file:
https://github.com/icestudent/vc-19-changes/blob/master/vadefs.h
There are quite a few libraries that also use _UINTPTR_T_DEFINED in the way I used it.
https://www.google.de/search?q=%22_UINTPTR_T_DEFINED%22
See libSDL for instance:
https://hg.libsdl.org/SDL/file/9e13f3286831/include/SDL_config_windows.h#l42
This one also wraps it with
#if !defined(STDINT_H) && (!defined(HAVE_STDINT_H) || !_HAVE_STDINT_H)
This seems to be a real mess.
What if we do not typedef uintptr_t and use our own name instead?
Something like this:
#ifdef _MSC_VER
#ifndef _MSC_STDINT_H_
#if _MSC_VER < 1300
typedef unsigned char uint8_t;
typedef unsigned short uint16_t;
typedef unsigned int uint32_t;
#else
typedef unsigned __int8 uint8_t;
typedef unsigned __int16 uint16_t;
typedef unsigned __int32 uint32_t;
#endif
#endif
#if _MSC_VER < 1300
#ifdef _WIN64
typedef unsigned long long __pyx_uintptr_t;
#else
typedef unsigned int __pyx_uintptr_t;
#endif
#else
#ifdef _WIN64
typedef unsigned __int64 __pyx_uintptr_t;
#else
typedef unsigned __int32 __pyx_uintptr_t;
#endif
#endif
#else
#include <stdint.h>
typedef uintptr_t __pyx_uintptr_t;
#endif
...
#define __Pyx_PyObject_AsWritableString(s) ((char*)(__pyx_uintptr_t) __Pyx_PyObject_AsString(s))
...
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.
Should I redo the pull request with the naming change?
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.
Yeah, I think that looks safer. Doesn't help users who need this, but also doesn't interfere with whatever solution they find for themselves (which they probably need to do anyway).
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 updated the commit. Please let me know if there's anything wrong with it.
Cython/Utility/ModuleSetupCode.c
Outdated
@@ -260,12 +260,30 @@ | |||
#ifndef _MSC_STDINT_H_ | |||
#if _MSC_VER < 1300 | |||
typedef unsigned char uint8_t; | |||
typedef unsigned short uint16_t; |
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.
Thanks for cleaning this up.
The reasoning behind this change was: |
This avoids the compiler warning warning: cast discards ‘const’ qualifier from pointer target type [-Wcast-qual] by casting the pointer to uintptr_t before removing the const-ness. The warning was introduced in 6cd70f0 Also merge the two places that define stdint types.
#else | ||
typedef unsigned __int8 uint8_t; | ||
typedef unsigned __int32 uint32_t; | ||
typedef unsigned __int8 uint8_t; |
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.
Yep, that's the intended indentation. :)
This avoids the compiler warning
warning: cast discards ‘const’ qualifier from pointer target type [-Wcast-qual]
by casting the pointer to uintptr_t before removing the const-ness.
The warning was introduced in 6cd70f0
Also merge the two places that define stdint types.