Created attachment 82668 [details] [review] fix of gcc 4.8.1 warnings gcc 4.8.1 warning fixes
Created attachment 82669 [details] [review] gcc 4.8.1 cross compiling warning fixes
Comment on attachment 82668 [details] [review] fix of gcc 4.8.1 warnings Review of attachment 82668 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-sysdeps-win.c @@ +173,4 @@ > > if ((result = GetExtendedTcpTable (tcp_table, &size, TRUE, AF_INET, TCP_TABLE_OWNER_PID_ALL, 0)) != NO_ERROR) > { > + _dbus_verbose ("Error fetching tcp table %d\n", (int)result); If result is a DWORD, I'd prefer "... %lu\n", (unsigned long) result). Trivial: space after cast, please. I was going to suggest using _dbus_win_error_string(), but it seems that does a malloc or equivalent, which seems too complicated for a _dbus_verbose(). @@ +187,4 @@ > result = p->dwOwningPid; > } > > + _dbus_verbose ("got pid %d\n", (int)result); Similarly, "got pid %lu\n", (unsigned long) result) seems better? (DWORD is always unsigned 32-bit, which is no larger than unsigned long, if I understand the Windows type system correctly.) @@ +882,4 @@ > > if (!OpenProcessToken (process_handle, TOKEN_QUERY, &process_token)) > { > + _dbus_verbose("OpenProcessToken failed with error %d", (int)GetLastError()); Same here @@ +890,4 @@ > || (token_user = alloca (n)) == NULL > || !GetTokenInformation (process_token, TokenUser, token_user, n, &n)) > { > + _dbus_verbose("GetTokenInformation failed with error %d", (int)GetLastError ()); Same here @@ +2131,4 @@ > if (!CryptAcquireContext (&hprov, NULL, NULL, PROV_RSA_FULL, CRYPT_VERIFYCONTEXT)) > return FALSE; > > + if (!CryptGenRandom (hprov, n_bytes, (BYTE *)p)) OK, fine. (Trivial: space after cast please: (BYTE *) p) @@ +2152,4 @@ > _dbus_get_tmpdir(void) > { > static const char* tmpdir = NULL; > + static unsigned char buf[1000]; OK, but don't you need "tmpdir = (char *) buf" instead of "tmpdir = buf" now? I notice this function isn't thread-safe... but that's something for another bug. @@ +2156,4 @@ > > if (tmpdir == NULL) > { > + unsigned char *last_slash; OK.
Comment on attachment 82669 [details] [review] gcc 4.8.1 cross compiling warning fixes Review of attachment 82669 [details] [review]: ----------------------------------------------------------------- ::: cmake/config.h.cmake @@ -19,5 @@ > #cmakedefine DBUS_MACHINE_UUID_FILE "@DBUS_MACHINE_UUID_FILE@" > #cmakedefine DBUS_DAEMONDIR "@DBUS_DAEMONDIR@" > #cmakedefine PACKAGE "@PACKAGE@" > -/* Version number of package */ > -#cmakedefine DBUS_MAJOR_VERSION @DBUS_MAJOR_VERSION@ Where do these get defined, if not here?
(In reply to comment #3) > Comment on attachment 82669 [details] [review] [review] > gcc 4.8.1 cross compiling warning fixes > > Review of attachment 82669 [details] [review] [review]: > ----------------------------------------------------------------- > > Where do these get defined, if not here? > > #cmakedefine DBUS_MACHINE_UUID_FILE "@DBUS_MACHINE_UUID_FILE@" tools dbus-launch.c -> unix only need to exclude from patch, otherwise it will break cmake linux build > > #cmakedefine DBUS_DAEMONDIR "@DBUS_DAEMONDIR@" not used by any c file > > #cmakedefine PACKAGE "@PACKAGE@" not used by any c file > > -/* Version number of package */ > > -#cmakedefine DBUS_MAJOR_VERSION @DBUS_MAJOR_VERSION@ is defined in dbus/dbus-sysdeps.h.in
(In reply to comment #2) > Comment on attachment 82668 [details] [review] [review] > fix of gcc 4.8.1 warnings > > Review of attachment 82668 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: dbus/dbus-sysdeps-win.c > @@ +173,4 @@ > > > > if ((result = GetExtendedTcpTable (tcp_table, &size, TRUE, AF_INET, TCP_TABLE_OWNER_PID_ALL, 0)) != NO_ERROR) > > { > > + _dbus_verbose ("Error fetching tcp table %d\n", (int)result); > > If result is a DWORD, I'd prefer "... %lu\n", (unsigned long) result). > Because DWORD is already defined unsigned long /usr/i686-w64-mingw32/sys-root/mingw/include/mqoai.h:371: typedef unsigned __LONG32 DWORD; /usr/i686-w64-mingw32/sys-root/mingw/include/mapidbg.h:36:typedef unsigned __LONG32 DWORD; /usr/i686-w64-mingw32/sys-root/mingw/include/mapinls.h:27: typedef unsigned __LONG32 DWORD; /usr/i686-w64-mingw32/sys-root/mingw/include/winsmcrd.h:29: typedef ULONG DWORD; /usr/i686-w64-mingw32/sys-root/mingw/include/ddk/ntstrsafe.h:32:typedef ULONG DWORD; /usr/i686-w64-mingw32/sys-root/mingw/include/wtypes.h:74:typedef ULONG DWORD; /usr/i686-w64-mingw32/sys-root/mingw/include/windef.h:117:typedef unsigned __LONG32 DWORD; it would be enough to use prefer "... %lu\n", result) i guess ?
(In reply to comment #2) > Comment on attachment 82668 [details] [review] [review] > fix of gcc 4.8.1 warnings > > Review of attachment 82668 [details] [review] [review]: > ----------------------------------------------------------------- > @@ +2152,4 @@ > > _dbus_get_tmpdir(void) > > { > > static const char* tmpdir = NULL; > > + static unsigned char buf[1000]; > > OK, but don't you need "tmpdir = (char *) buf" instead of "tmpdir = buf" now? > not sure, I only changed the types as the compiler suggested. > I notice this function isn't thread-safe... but that's something for another > bug. Which will probably refactor this lines too, so i will remove them from this patch.
(In reply to comment #2) > Comment on attachment 82668 [details] [review] [review] > fix of gcc 4.8.1 warnings > > Review of attachment 82668 [details] [review] [review]: > ----------------------------------------------------------------- > > I was going to suggest using _dbus_win_error_string(), but it seems that > does a malloc or equivalent, which seems too complicated for a > _dbus_verbose(). _dbus_win_warn_win_error requires a type cast too, void _dbus_win_warn_win_error (const char *message, int code) I suggest to add a generic printf style like _dbus_error() and/or dbus_warning() function. May also help on other places
(In reply to comment #7) > I suggest to add a generic printf style like _dbus_error() and/or > dbus_warning() function. May also help on other places Update: dbus_warn() is also present, dbus_error() not.
(In reply to comment #5) > > If result is a DWORD, I'd prefer "... %lu\n", (unsigned long) result). > > > Because DWORD is already defined unsigned long ... > it would be enough to use prefer "... %lu\n", result) i guess ? I thought you said on another bug that __LONG32 is sometimes int? If DWORD is always *the same size as* unsigned long (as opposed to *no larger than* unsigned long), then I'm fine with printf'ing it as a %lu with no cast. If DWORD could have a size differing from unsigned long, then you still need to cast it. I doubt Microsoft is particularly likely to release a Windows version that's LP64 (32-bit int, 64-bit long and pointer - like x86-64 Linux) since they seem to have settled on LLP64 (32-bit int and long, 64-bit "long long" and pointer) instead, but you never know.
(In reply to comment #7) > _dbus_win_warn_win_error requires a type cast too, > > void _dbus_win_warn_win_error (const char *message, int code) Yeah, that second parameter should be DWORD, if the intention is "takes a Windows system error code, as if from GetLastError()".
(In reply to comment #10) > (In reply to comment #7) > > _dbus_win_warn_win_error requires a type cast too, > > > > void _dbus_win_warn_win_error (const char *message, int code) > > Yeah, that second parameter should be DWORD, if the intention is "takes a > Windows system error code, as if from GetLastError()". and it does not support printf style parameter list. :-(
(In reply to comment #9) > (In reply to comment #5) > > > If result is a DWORD, I'd prefer "... %lu\n", (unsigned long) result). > > > > > Because DWORD is already defined unsigned long > ... > > it would be enough to use prefer "... %lu\n", result) i guess ? > > I thought you said on another bug that __LONG32 is sometimes int? > > If DWORD is always *the same size as* unsigned long (as opposed to *no > larger than* unsigned long), then I'm fine with printf'ing it as a %lu with > no cast. At https://bugs.freedesktop.org/show_bug.cgi?id=61874#c5 there is mentioned that on 64 bit cygwin systems long is 64 bit, so int is taken which is 32bit on that platform, but DWORD is always unsigned "32bits" > > If DWORD could have a size differing from unsigned long, then you still need > to cast it. sure, but there is not GetLastWin64Error or similar. >I doubt Microsoft is particularly likely to release a Windows > version that's LP64 (32-bit int, 64-bit long and pointer - like x86-64 > Linux) or cygwin 64bit as mentioned above
(In reply to comment #12) > At https://bugs.freedesktop.org/show_bug.cgi?id=61874#c5 there is mentioned > that on 64 bit cygwin systems long is 64 bit, so int is taken which is 32bit > on that platform, but DWORD is always unsigned "32bits" Ah, I had half-remembered that there was some sort of counter-example. If we use dbus-sysdeps-win on Cygwin (do we? it isn't clear to me...) then yes I'm afraid it will need the (unsigned long) cast.
Created attachment 84720 [details] [review] Fixed gcc-4.8.1-Wformat warnings on windows
(In reply to comment #13) > (In reply to comment #12) > > At https://bugs.freedesktop.org/show_bug.cgi?id=61874#c5 there is mentioned > > that on 64 bit cygwin systems long is 64 bit, so int is taken which is 32bit > > on that platform, but DWORD is always unsigned "32bits" > > Ah, I had half-remembered that there was some sort of counter-example. > > If we use dbus-sysdeps-win on Cygwin (do we? it isn't clear to me...) no, cygwin is unix like.
Comment on attachment 84720 [details] [review] Fixed gcc-4.8.1-Wformat warnings on windows Review of attachment 84720 [details] [review]: ----------------------------------------------------------------- Sure, looks good, since you said Cygwin doesn't use these files.
(In reply to comment #4) > (In reply to comment #3) > > Comment on attachment 82669 [details] [review] [review] [review] > > gcc 4.8.1 cross compiling warning fixes > > > > Review of attachment 82669 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > Where do these get defined, if not here? > > > > #cmakedefine DBUS_MACHINE_UUID_FILE "@DBUS_MACHINE_UUID_FILE@" > tools dbus-launch.c -> unix only > need to exclude from patch, otherwise it will break cmake linux build > > > > #cmakedefine DBUS_DAEMONDIR "@DBUS_DAEMONDIR@" > not used by any c file > > > > #cmakedefine PACKAGE "@PACKAGE@" > not used by any c file > > > > -/* Version number of package */ > > > -#cmakedefine DBUS_MAJOR_VERSION @DBUS_MAJOR_VERSION@ > is defined in dbus/dbus-sysdeps.h.in forget the comment about the first three entries > #cmakedefine DBUS_MACHINE_UUID_FILE "@DBUS_MACHINE_UUID_FILE@" > #cmakedefine DBUS_DAEMONDIR "@DBUS_DAEMONDIR@" > #cmakedefine PACKAGE "@PACKAGE@" they are not changed. Only the version related defines has been changed -/* Version number of package */ -#cmakedefine DBUS_MAJOR_VERSION @DBUS_MAJOR_VERSION@ -#cmakedefine DBUS_MINOR_VERSION @DBUS_MINOR_VERSION@ -#cmakedefine DBUS_MICRO_VERSION @DBUS_MICRO_VERSION@ -#cmakedefine DBUS_VERSION ((@DBUS_MAJOR_VERSION@ << 16) | (@DBUS_MINOR_VERSION@ << 8) | (@DBUS_MICRO_VERSION@)) -#cmakedefine DBUS_VERSION_STRING "@DBUS_VERSION_STRING@" -#cmakedefine DBUS_ENABLE_STATS -#define VERSION DBUS_VERSION_STRING +#cmakedefine DBUS_ENABLE_STATS which are defined in dbus/dbus-sysdeps.h.in. Any more problems ?
Comment on attachment 84720 [details] [review] Fixed gcc-4.8.1-Wformat warnings on windows applied to master
(In reply to comment #17) > which are defined in dbus/dbus-sysdeps.h.in Where do you get that file? I don't see one in git master.
(In reply to comment #19) > (In reply to comment #17) > > which are defined in dbus/dbus-sysdeps.h.in > > Where do you get that file? I don't see one in git master. s/sys/arch-/g
Comment on attachment 82669 [details] [review] gcc 4.8.1 cross compiling warning fixes Review of attachment 82669 [details] [review]: ----------------------------------------------------------------- (In reply to comment #20) > s/sys/arch-/g Ah, that makes more sense :-) The DBUS_*VERSION* stuff can be deleted, but you need to either keep VERSION expanding to DBUS_VERSION_STRING, or replace all its uses (which seem to be in tools/*.c - use git grep for '\<VERSION\>'). As a general principle, things should be (conditionally) defined in config.h.cmake if and only if they are (conditionally) AC_DEFINE'd in configure.ac (which results in them going in config.h.in and hence config.h). However, the Autotools do define a few things automatically, such as PACKAGE and VERSION, so not everything is immediately visible.
(In reply to comment #21) > either keep VERSION > expanding to DBUS_VERSION_STRING, or replace all its uses ... or having VERSION expand to "@DBUS_VERSION_STRING@" directly would be fine too - that's the closest equivalent of what the Autotools do (VERSION expands to "1.7.5" or whatever, without going via an intermediate macro).
Created attachment 84788 [details] [review] Generate autotools provided defines using a cmake macro I was inpirated by your last comments to refactor generating of VERSION and PACKAGE related macros.
Comment on attachment 84788 [details] [review] Generate autotools provided defines using a cmake macro Review of attachment 84788 [details] [review]: ----------------------------------------------------------------- Nice! OK, please commit.
Comment on attachment 84788 [details] [review] Generate autotools provided defines using a cmake macro applied to master
Comment on attachment 82669 [details] [review] gcc 4.8.1 cross compiling warning fixes applied to master
All fixed in git for 1.7.6, I think.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.