zopefoundation / persistent

automatic persistence for Python objects
https://pypi.org/project/persistent/
Other
46 stars 28 forks source link

Fix C compilation warning #111

Closed mgedmin closed 5 years ago

mgedmin commented 5 years ago

gcc 8.3.0 emits this during python setup.py build:

persistent/cPersistence.c: In function ‘Per_repr’:
persistent/cPersistence.c:1481:44: warning: format ‘%llx’ expects argument of type ‘long long unsigned int’, but argument 4 has type ‘uint64_t’ {aka ‘long unsigned int’} [-Wformat=]
         snprintf(buf, sizeof(buf) - 1, "%llx", oid_value);
                                         ~~~^   ~~~~~~~~~
                                         %lx

\<stdint.h> \<inttypes.h> defines standard macros such as PRId64, PRIu64 and PRIx64 for printing {u,}int64_t values, so let's use them.

(Python 3.4 on Windows uses a version of MSVC that doesn't support C99 and has no \<inttypes.h>. Luckily Python 3.4 is EOL and we can drop it.)

jamadden commented 5 years ago

I can confirm this builds on macOS 10.14 with both Python 2.7 and Python 3.7 just fine. However it is failing on Python 3.4 and 3.5 on Appveyor:

c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\BIN\cl.exe /c /nologo /Ox /MD /W3 /GS- /DNDEBUG -Ic:\python34\include -Ic:\python34\include /Tcpersistent/cPersistence.c /Fobuild\temp.win32-3.4\Release\persistent/cPersistence.obj
cPersistence.c
persistent/cPersistence.c(1482) : error C2146: syntax error : missing ')' before identifier 'PRIx64'
persistent/cPersistence.c(1482) : error C2059: syntax error : ')'
error: command 'c:\\Program Files (x86)\\Microsoft Visual Studio 10.0\\VC\\BIN\\cl.exe' failed with exit status 2

3.4 is easy: just drop it. But the same error exists for 3.5.

jamadden commented 5 years ago

Maybe

#if !defined(PY3K) && defined(_WIN32)
  typedef unsigned long long uint64_t;
#else
  #include <stdint.h>
#endif
#ifndef  PRlx64
  #define PRIx64 "llx"
#endif

?

jamadden commented 5 years ago

From what I gather, that define is actually in <inttypes.h>, and I don't think <stdint.h> is required to include that?

mgedmin commented 5 years ago

From what I gather, that define is actually in , and I don't think is required to include that?

Uhh, did I conflate C headers? Back to StackOverflow I go...

mgedmin commented 5 years ago

Yeah, the answer I cribbed does #include <inttypes.h>, which I totally mixed up with <stdint.h> in my mind.

mgedmin commented 5 years ago

(Now I'm left uncertain if I should attempt to #define __STDC_FORMAT_MACROS before I include <inttypes.h> or not.)

jamadden commented 5 years ago

(Now I'm left uncertain if I should attempt to #define __STDC_FORMAT_MACROS before I include or not.)

I don't think that's necessary. Unless it's a MS thing.

jamadden commented 5 years ago

Sigh. So that fixes 3.5 on Appveyor, but breaks 3.4:

 c:\Program Files (x86)\Microsoft Visual Studio 10.0\VC\BIN\cl.exe /c /nologo /Ox /MD /W3 /GS- /DNDEBUG -Ic:\python34\include -Ic:\python34\include /Tcpersistent/cPersistence.c /Fobuild\temp.win32-3.4\Release\persistent/cPersistence.obj
cPersistence.c
persistent/cPersistence.c(37) : fatal error C1083: 
    Cannot open include file: 'inttypes.h': No such file or directory
error: command 'c:\\Program Files (x86)\\Microsoft Visual Studio 10.0\\VC\\BIN\\cl.exe' failed with exit status 2

Time to just drop 3.4? The alternative I see is to do a more complicated PY_VERSION (or whatever that macro is called) check for the custom definition of PR....

mgedmin commented 5 years ago

I don't think that's necessary. Unless it's a MS thing.

It seems to be a C++ thing? The stackoverflow comments are confusing.

mgedmin commented 5 years ago

Dropped Python 3.4, rebased on master to avoid merge conflicts in the changelog.

jamadden commented 5 years ago

It seems to be a C++ thing?

But on C++ wouldn't you include <cinttypes>?

jamadden commented 5 years ago

Note this partially obviates (and conflicts with) #110