Created attachment 120017 [details] [review] Fix several -Wpointer-sign warnings in test_hex_roundtrip() and related types.
Created attachment 120018 [details] [review] Fix 'warning: pointer targets in assignment differ in signedness [-Wpointer-sign]'.
Created attachment 120019 [details] error log file
Created attachment 120020 [details] [review] Fix 'warning: pointer targets in assignment differ in signedness [-Wpointer-sign]'.
Created attachment 120021 [details] [review] Fix warning: "pointer targets in passing argument 5 of 'byteswap_body_helper' differ in signedness [-Wpointer-sign]".
Created attachment 120022 [details] [review] Fix warning: "pointer targets in passing argument 2 of '_dbus_string_init_const_len' differ in signedness [-Wpointer-sign]".
Created attachment 120023 [details] [review] Fix warning: "pointer targets in passing argument 2 of '_dbus_unpack_uint32' differ in signedness [-Wpointer-sign]".
Created attachment 120024 [details] error log file (remaining warnings)
Created attachment 120027 [details] [review] Fix warning: "pointer targets in assignment differ in signedness [-Wpointer-sign]". contains all related warnings
Created attachment 120028 [details] [review] Fix warning: "pointer targets in passing argument 3 of 'dbus_connection_get_adt_audit_session_data' differ in signedness [-Wpointer-sign]".
Created attachment 120029 [details] [review] Fix warning: "pointer targets in passing argument 3 of 'pack_[248]_octets' differ in signedness [-Wpointer-sign]".
Created attachment 120030 [details] [review] Fix warning: "pointer targets in passing argument 2 of '_dbus_string_append_len' differ in signedness [-Wpointer-sign]".
Created attachment 120031 [details] [review] Fix warning: "pointer targets in passing argument 1 of 'check_sha_binary' differ in signedness [-Wpointer-sign]".
Created attachment 120032 [details] [review] Fix warning: "pointer targets in passing argument 4 of 'marshal_len_followed_by_bytes' differ in signedness [-Wpointer-sign]".
Created attachment 120033 [details] [review] Fix warning: "pointer targets in passing argument 1 of '_dbus_verbose_bytes' differ in signedness [-Wpointer-sign]".
Created attachment 120034 [details] [review] Fix warning: "pointer targets in assignment differ in signedness [-Wpointer-sign]".
Created attachment 120035 [details] error log file (remaining warnings; update)
*** Bug 89284 has been marked as a duplicate of this bug. ***
Comment on attachment 120017 [details] [review] Fix several -Wpointer-sign warnings in test_hex_roundtrip() and related types. Review of attachment 120017 [details] [review]: ----------------------------------------------------------------- Only suitable for master in any case ::: dbus/dbus-string-util.c @@ +120,5 @@ > #include <stdio.h> > > static void > +test_hex_roundtrip (const char *data, > + int len) OK @@ +170,5 @@ > _dbus_string_free (&decoded); > } > > +typedef void (* TestRoundtripFunc) (const char *data, > + int len); OK @@ +194,4 @@ > (* func) ("1234", 5); > (* func) ("12345", 6); > { > + char buf[512]; This changes the meaning of the assignment buf[i] = i: instead of being an out-of-range unsigned value (unsigned wraparound is well-defined), it's out-of-range signed (signed wraparound is undefined, and I think the same might be true for signed coercion). I'd prefer unsigned char buf[512]; while (...) { buf[i] = (i & 0xFF); ... } while (...) { (* func) ((unsigned char *) buf, i); } In this case the cast is justified.
Comment on attachment 120021 [details] [review] Fix warning: "pointer targets in passing argument 5 of 'byteswap_body_helper' differ in signedness [-Wpointer-sign]". Review of attachment 120021 [details] [review]: ----------------------------------------------------------------- OK for master
Comment on attachment 120022 [details] [review] Fix warning: "pointer targets in passing argument 2 of '_dbus_string_init_const_len' differ in signedness [-Wpointer-sign]". Review of attachment 120022 [details] [review]: ----------------------------------------------------------------- Basically OK for master. I'd slightly prefer casting to (const char *) in all cases, to make it clearer that we aren't casting away constness (in C++ terms: we're doing a reinterpret_cast<> but not a const_cast<>).
Comment on attachment 120022 [details] [review] Fix warning: "pointer targets in passing argument 2 of '_dbus_string_init_const_len' differ in signedness [-Wpointer-sign]". Review of attachment 120022 [details] [review]: ----------------------------------------------------------------- Basically OK for master. I'd slightly prefer casting to (const char *) in all cases, to make it clearer that we aren't casting away constness (in C++ terms: we're doing a reinterpret_cast<> but not a const_cast<>). ::: dbus/dbus-sha.c @@ +524,4 @@ > DBusString expected_str; > DBusString results; > > + _dbus_string_init_const_len (&input_str, (char *) input, input_len); In this one, input is definitely const, so we should definitely prefer to cast to (const char *).
Comment on attachment 120023 [details] [review] Fix warning: "pointer targets in passing argument 2 of '_dbus_unpack_uint32' differ in signedness [-Wpointer-sign]". Review of attachment 120023 [details] [review]: ----------------------------------------------------------------- OK. I wonder whether we should have static inline const unsigned char * _dbus_string_get_const_udata (const DBusString *str) { return (const unsigned char *) _dbus_string_get_const_data (str); } and the same for _dbus_string_get_const_data_len, and maybe even their non-const equivalents if it would be useful.
Comment on attachment 120028 [details] [review] Fix warning: "pointer targets in passing argument 3 of 'dbus_connection_get_adt_audit_session_data' differ in signedness [-Wpointer-sign]". Review of attachment 120028 [details] [review]: ----------------------------------------------------------------- OK for master.
Comment on attachment 120029 [details] [review] Fix warning: "pointer targets in passing argument 3 of 'pack_[248]_octets' differ in signedness [-Wpointer-sign]". Review of attachment 120029 [details] [review]: ----------------------------------------------------------------- OK for master
Comment on attachment 120030 [details] [review] Fix warning: "pointer targets in passing argument 2 of '_dbus_string_append_len' differ in signedness [-Wpointer-sign]". Review of attachment 120030 [details] [review]: ----------------------------------------------------------------- > Bug: https://bugs.freedesktop.org/attachment.cgi?id=120024 Please double-check this when you add the Reviewed-by - it should be https://bugs.freedesktop.org/show_bug.cgi?id=93069 I'd slightly prefer (const char *) but this is fine for maste either way.
Comment on attachment 120031 [details] [review] Fix warning: "pointer targets in passing argument 1 of 'check_sha_binary' differ in signedness [-Wpointer-sign]". Review of attachment 120031 [details] [review]: ----------------------------------------------------------------- Fine for master
Comment on attachment 120032 [details] [review] Fix warning: "pointer targets in passing argument 4 of 'marshal_len_followed_by_bytes' differ in signedness [-Wpointer-sign]". Review of attachment 120032 [details] [review]: ----------------------------------------------------------------- I'd prefer (const unsigned char *), feel free to commit to master with that change (and the bug number instead of some attachment number)
Comment on attachment 120033 [details] [review] Fix warning: "pointer targets in passing argument 1 of '_dbus_verbose_bytes' differ in signedness [-Wpointer-sign]". Review of attachment 120033 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-marshal-basic.c @@ +1386,4 @@ > > d = _dbus_string_get_const_data_len (str, start, len); > > + _dbus_verbose_bytes ((unsigned char *) d, len, start); const please @@ +1592,2 @@ > _dbus_verbose ("READ DATA\n"); \ > + _dbus_verbose_bytes ((unsigned char*)v_ARRAY_##typename, sizeof (literal), 0); \ Constness and whitespace: (const unsigned char *) whatever Fine for commit to master with that change.
Comment on attachment 120034 [details] [review] Fix warning: "pointer targets in assignment differ in signedness [-Wpointer-sign]". Review of attachment 120034 [details] [review]: ----------------------------------------------------------------- This one is more subtle and some cases need some alteration. ::: dbus/dbus-marshal-basic.c @@ +1560,4 @@ > if (!_dbus_marshal_write_basic (&str, pos, DBUS_TYPE_UINT32, &v_UINT32, \ > byte_order, &next)) \ > _dbus_assert_not_reached ("no memory"); \ > + v_ARRAY_##typename = (typename *) literal; \ OK, but only because I inspected the callers (it isn't trivially obvious that it's correct). I'd slightly prefer to have twice as many array "literals": const dbus_int16_t array2[3] = { -42, 23, 0 }; const dbus_uint16_t array2u[3] = { 124, 457, 780 }; and use the right one for the test - as a bonus that would also mean we're exercising negative numbers for the signed types. ::: dbus/dbus-marshal-header.c @@ +1462,2 @@ > > flags_p = _dbus_string_get_data_len (&header->data, FLAGS_OFFSET, 1); Leave it unsigned, and cast the result of _dbus_string_get_const_data_len instead: we're doing bitwise operations on *flags_p, which effectively treats it as unsigned anyway. @@ +1479,4 @@ > _dbus_header_get_flag (DBusHeader *header, > dbus_uint32_t flag) > { > + const char *flags_p; Leave it unsigned, and cast the result of _dbus_string_get_const_data_len instead: we're doing bitwise operations on *flags_p, which effectively treats it as unsigned anyway. Actually, I think this (but not the one above) would be clearer as: unsigned char flags = _dbus_string_get_byte (&header->data, FLAGS_OFFSET); return (flags & flag) != 0; No casts! :-) ::: dbus/dbus-marshal-recursive.c @@ +341,5 @@ > _dbus_type_signature_next (const char *type_str, > int *type_pos) > { > + const char *p; > + const char *start; I think I'd prefer to cast like you (mostly) did below, so we're consistently treating the signatures as bytestrings and not text in all these functions. @@ +855,4 @@ > { > _dbus_assert (!reader->klass->types_only); > > + *value_location = (unsigned char *) _dbus_string_get_const_data_len (reader->value_str, const ::: dbus/dbus-marshal-validate.c @@ +52,5 @@ > int type_pos, > int len) > { > + const char *p; > + const char *end; I think I'd prefer to cast the result of _dbus_string_get_const_data_len() like you did below, so we're consistently treating the signatures as bytestrings and not text in all these functions. @@ +725,4 @@ > _dbus_type_reader_init_types_only (&reader, > expected_signature, expected_signature_start); > > + p = (unsigned char *) _dbus_string_get_const_data_len (value_str, value_pos, len); (const unsigned char *) @@ +800,4 @@ > if (len == 0) > return FALSE; > > + s = (unsigned char *) _dbus_string_get_const_data (str) + start; (const unsigned char *) @@ +941,4 @@ > return FALSE; > > last_dot = NULL; > + iface = (unsigned char *) _dbus_string_get_const_data (str) + start; (const unsigned char *) @@ +1015,4 @@ > if (len == 0) > return FALSE; > > + member = (unsigned char *) _dbus_string_get_const_data (str) + start; (const unsigned char *) @@ +1107,4 @@ > return FALSE; > > last_dot = NULL; > + iface = (unsigned char *) _dbus_string_get_const_data (str) + start; (const unsigned char *) ::: dbus/dbus-sysdeps-win.c @@ +603,2 @@ > > + data1 = _dbus_string_get_const_data_len (buffer1, start1, len1); Looks like a mistake? You've just moved blank lines around? ::: tools/dbus-spam.c @@ +245,4 @@ > n_random_sizes++; > } > > + random_sizes = (unsigned int *) dbus_new0 (int, n_random_sizes); random_sizes = dbus_new0 (unsigned int, n_random_sizes);
Comment on attachment 120035 [details] error log file (remaining warnings; update) > if (!CryptGenRandom (hprov, n_bytes, p)) > WINIMPM WINBOOL WINAPI CryptGenRandom (HCRYPTPROV hProv, DWORD dwLen, BYTE *pbBuffer); Make "char *p" unsigned, and cast the result of _dbus_string_get_data_len() (or add _dbus_string_get_udata_len() if you prefer) > last_slash = _mbsrchr (buf, '\\'); > _CRTIMP _CONST_RETURN unsigned char *__cdecl _mbsrchr(const unsigned char *_Str,unsigned int _Ch); I'm surprised that Windows has string functions that take unsigned chars... a string of characters is sort of the point here. I think you might need some casts unfortunately. >/home/ralf/src/dbus-1/dbus/dbus-sysdeps-win.c: In function 'dump_backtrace_for_thread': >/home/ralf/src/dbus-1/dbus/dbus-sysdeps-win.c:2511:22: warning: unknown conversion type character 'l' in format [-Wformat=] > DPRINTF ("%3d %s+0x%llx", i++, pSymbol->Name, displacement); Does this need to become 0x%I64x? >/home/ralf/src/dbus-1/bus/config-parser.c:3544:1: warning: 'test_default_system_servicedirs' defined but not used [-Wunused-function] > test_default_system_servicedirs (void) #ifndef DBUS_WIN would make sense: we deliberately don't test (or use, or support) the system bus on Windows. >/home/ralf/src/dbus-1/bus/dispatch.c:1383:1: warning: 'check_get_connection_unix_process_id' defined but not used [-Wunused-function] > check_get_connection_unix_process_id (BusContext *context, I thought you fixed this on another branch, by running that test even on Windows? >/home/ralf/src/dbus-1/bus/dispatch.c:4956:1: warning: 'bus_dispatch_test_conf_fail' defined but not used [-Wunused-function] > bus_dispatch_test_conf_fail (const DBusString *test_data_dir, This is also system-bus-related and non-Windows (it's the launch-helper, which relies on Unix setuid semantics), so we can #ifdef it out.
If we want to fix -Wpointer-sign (which I think we do - thank you for doing all these fixes) then we should move it out from the "warnings we don't want" list to the "warnings we do want" list in configure.ac. However, we can't do that until a native Linux build works in that configuration, which will probably require a few fixes in Unix- and Linux-specific code.
Comment on attachment 120017 [details] [review] Fix several -Wpointer-sign warnings in test_hex_roundtrip() and related types. committed with mentioned fixes to master
Comment on attachment 120021 [details] [review] Fix warning: "pointer targets in passing argument 5 of 'byteswap_body_helper' differ in signedness [-Wpointer-sign]". committed to master
Comment on attachment 120022 [details] [review] Fix warning: "pointer targets in passing argument 2 of '_dbus_string_init_const_len' differ in signedness [-Wpointer-sign]". committed to master with mentioned (const char *) typecast
Comment on attachment 120028 [details] [review] Fix warning: "pointer targets in passing argument 3 of 'dbus_connection_get_adt_audit_session_data' differ in signedness [-Wpointer-sign]". committed to master
Comment on attachment 120029 [details] [review] Fix warning: "pointer targets in passing argument 3 of 'pack_[248]_octets' differ in signedness [-Wpointer-sign]". committed to master with bug id fix.
Comment on attachment 120032 [details] [review] Fix warning: "pointer targets in passing argument 4 of 'marshal_len_followed_by_bytes' differ in signedness [-Wpointer-sign]". committed to master with (const char *) typecast
Comment on attachment 120031 [details] [review] Fix warning: "pointer targets in passing argument 1 of 'check_sha_binary' differ in signedness [-Wpointer-sign]". committed to master
Comment on attachment 120033 [details] [review] Fix warning: "pointer targets in passing argument 1 of '_dbus_verbose_bytes' differ in signedness [-Wpointer-sign]". committed to master with mentioned (const unsigned char *) typecast
Comment on attachment 120030 [details] [review] Fix warning: "pointer targets in passing argument 2 of '_dbus_string_append_len' differ in signedness [-Wpointer-sign]". committed to master with const cast prefix and fixed id
Comment on attachment 120030 [details] [review] Fix warning: "pointer targets in passing argument 2 of '_dbus_string_append_len' differ in signedness [-Wpointer-sign]". committed to master with const cast prefix
Created attachment 120055 [details] [review] Fix 'warning: pointer targets in assignment differ in signedness [-Wpointer-sign]'.
Created attachment 120056 [details] [review] Fix warning: "pointer targets in passing argument 2 of '_dbus_unpack_uint32' differ in signedness [-Wpointer-sign]".
Comment on attachment 120055 [details] [review] Fix 'warning: pointer targets in assignment differ in signedness [-Wpointer-sign]'. Review of attachment 120055 [details] [review]: ----------------------------------------------------------------- ::: dbus/dbus-marshal-basic.c @@ +1560,4 @@ > if (!_dbus_marshal_write_basic (&str, pos, DBUS_TYPE_UINT32, &v_UINT32, \ > byte_order, &next)) \ > _dbus_assert_not_reached ("no memory"); \ > + v_ARRAY_##typename = (typename *) literal; \ This fails to compile on linux because the related types for example INT16 does not exist.
Comment on attachment 120055 [details] [review] Fix 'warning: pointer targets in assignment differ in signedness [-Wpointer-sign]'. Review of attachment 120055 [details] [review]: ----------------------------------------------------------------- Looks good, except for the one you already noticed doesn't compile on Linux ::: dbus/dbus-marshal-basic.c @@ +1560,4 @@ > if (!_dbus_marshal_write_basic (&str, pos, DBUS_TYPE_UINT32, &v_UINT32, \ > byte_order, &next)) \ > _dbus_assert_not_reached ("no memory"); \ > + v_ARRAY_##typename = (typename *) literal; \ Indeed. INT16 is not a type in any portable way (I'm a bit surprised if it exists on Windows). Please add more "literal" arrays like I suggested instead of adding this cast, perhaps something like: dbus_int16_t array2[3] = { 124, -457, 780 }; dbus_uint16_t array2u[3] = { 124, 457, 780 }; dbus_int32_t array4[3] = { 123, -456, 789 }; dbus_uint32_t array4u[3] = { 123, 456, 789 }; and change the "literal" parameter to this macro accordingly.
Comment on attachment 120056 [details] [review] Fix warning: "pointer targets in passing argument 2 of '_dbus_unpack_uint32' differ in signedness [-Wpointer-sign]". Review of attachment 120056 [details] [review]: ----------------------------------------------------------------- Looks good, conditional on applying the one that introduces that function first
Created attachment 120069 [details] [review] Fix 'warning: pointer targets in assignment differ in signedness [-Wpointer-sign]'. (update) - with fix mentioned in comment 46
Created attachment 120074 [details] [review] Fix warning: "pointer targets in passing argument 3 of 'CryptGenRandom' differ in signedness [-Wpointer-sign]".
Created attachment 120075 [details] [review] Fix warning: "unknown conversion type character 'l' in format [-Wformat=]".
Created attachment 120076 [details] [review] Fix warning: "pointer targets in passing argument 1 of '_mbsrchr' differ in signedness [-Wpointer-sign]".
Created attachment 120081 [details] [review] Fix 'warning: pointer targets in assignment differ in signedness [-Wpointer-sign]'. (update 2) - remove unused hunks + dbus_uint64_t array8u[3] = { DBUS_UINT64_CONSTANT (0x123ffffffff), + DBUS_UINT64_CONSTANT (0x456ffffffff), + DBUS_UINT64_CONSTANT (0x789ffffffff) }; ... + dbus_int64_t *v_ARRAY_UINT64;
Comment on attachment 120069 [details] [review] Fix 'warning: pointer targets in assignment differ in signedness [-Wpointer-sign]'. (update) Review of attachment 120069 [details] [review]: ----------------------------------------------------------------- Looks good
Comment on attachment 120074 [details] [review] Fix warning: "pointer targets in passing argument 3 of 'CryptGenRandom' differ in signedness [-Wpointer-sign]". Review of attachment 120074 [details] [review]: ----------------------------------------------------------------- Sure
Comment on attachment 120075 [details] [review] Fix warning: "unknown conversion type character 'l' in format [-Wformat=]". Review of attachment 120075 [details] [review]: ----------------------------------------------------------------- Yes
Comment on attachment 120076 [details] [review] Fix warning: "pointer targets in passing argument 1 of '_mbsrchr' differ in signedness [-Wpointer-sign]". Review of attachment 120076 [details] [review]: ----------------------------------------------------------------- OK
Comment on attachment 120081 [details] [review] Fix 'warning: pointer targets in assignment differ in signedness [-Wpointer-sign]'. (update 2) Review of attachment 120081 [details] [review]: ----------------------------------------------------------------- Still looks fine
Comment on attachment 120056 [details] [review] Fix warning: "pointer targets in passing argument 2 of '_dbus_unpack_uint32' differ in signedness [-Wpointer-sign]". committed to master
Comment on attachment 120069 [details] [review] Fix 'warning: pointer targets in assignment differ in signedness [-Wpointer-sign]'. (update) committed to master
Comment on attachment 120074 [details] [review] Fix warning: "pointer targets in passing argument 3 of 'CryptGenRandom' differ in signedness [-Wpointer-sign]". committed to master
Comment on attachment 120076 [details] [review] Fix warning: "pointer targets in passing argument 1 of '_mbsrchr' differ in signedness [-Wpointer-sign]". committed to master
Comment on attachment 120075 [details] [review] Fix warning: "unknown conversion type character 'l' in format [-Wformat=]". committed as part of http://cgit.freedesktop.org/dbus/dbus/commit/?id=90b751c28233856ba6749d2cc12381cdb288942c
Created attachment 120085 [details] linux autotools error log with enabled pointer-sign warnings
*** Bug 15522 has been marked as a duplicate of this bug. ***
Comment on attachment 120085 [details] linux autotools error log with enabled pointer-sign warnings Some of these (-Wunused-*) are because you're compiling without assertions, or possibly without checks or verbose mode.
(In reply to Simon McVittie from comment #65) > Comment on attachment 120085 [details] > linux autotools error log with enabled pointer-sign warnings > > Some of these (-Wunused-*) are because you're compiling without assertions, > or possibly without checks or verbose mode. I did a rebuild with ../dbus/configure -enable-asserts --enable-checks --enable-tests and enabled -Wpointer-sign warnings cflags: -Wall -Wextra -Wchar-subscripts -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wpointer-arith -Wpointer-sign -Wcast-align -Wno-address -Wfloat-equal -Wdeclaration-after-statement -Wno-missing-field-initializers -Wno-unused-parameter -Wno-type-limits -fno-common -fno-strict-aliasing -g -O2 and got: ../../dbus/dbus/dbus-transport.c: In function 'recover_unused_bytes': ../../dbus/dbus/dbus-transport.c:1058:11: warning: variable 'orig_len' set but not used [-Wunused-but-set-variable] int orig_len; ^ ../../dbus/bus/dir-watch-inotify.c: In function ‘_handle_inotify_watch’: ../../dbus/bus/dir-watch-inotify.c:62:7: warning: unused variable ‘i’ [-Wunused-variable] int i = 0; ^ I still need to instruct cmake to use the same flags to be in sync.
Created attachment 120143 [details] [review] Refactored cmake part dealing with compiler warnings to use warnings identifier.
Created attachment 120144 [details] [review] Keep cmake gcc builds in sync with autotools warnings.
Created attachment 120145 [details] [review] Enable -Wpointer-sign warnings for autotools and cmake.
Any time for review ?
(In reply to Ralf Habacker from comment #70) > Any time for review ? Please mark stuff for review as "patch" and "review?" - when I have time to work on D-Bus, the first thing I look at is the list of bugs in that state. I'll try to take a look at this soon.
Comment on attachment 120143 [details] [review] Refactored cmake part dealing with compiler warnings to use warnings identifier. Review of attachment 120143 [details] [review]: ----------------------------------------------------------------- Looks good ::: cmake/CMakeLists.txt @@ +189,5 @@ > + endif() > + > + set(WARNINGS "14018") > + set(WARNINGS_DISABLED "4127 4090 4101 4244") > + set(WARNINGS_ERRORS "4028 4013 4133 4047 4031 4002 4003 4114") Any chance you could put comments above these for what they actually do? Otherwise it's really hard to assess which ones are desirable and which ones aren't. # 14018: signed/unsigned mismatch set(WARNINGS "14018") # 4127: conditional expression is constant # 4090: discarding const or volatile without a cast # ... set(WARNINGS_DISABLED "4127 4090 ...")
Comment on attachment 120144 [details] [review] Keep cmake gcc builds in sync with autotools warnings. Review of attachment 120144 [details] [review]: ----------------------------------------------------------------- Looks OK, although this is going to become impossible if we start using the externally-curated list of warnings from <https://www.gnu.org/software/autoconf-archive/ax_compiler_flags.html>, which I've been quite tempted to do.
Comment on attachment 120145 [details] [review] Enable -Wpointer-sign warnings for autotools and cmake. Review of attachment 120145 [details] [review]: ----------------------------------------------------------------- Yes if it succeeds. I'll see what Travis-CI has to say about that: https://travis-ci.org/smcv/dbus-mirror/builds/108266544
Please stop putting "Reviewed-by" on git commits before I've reviewed them! A reader should be able to interpret Reviewed-by to mean "was actually reviewed by", not "I hope it will be reviewed by".
(In reply to Simon McVittie from comment #74) > Yes if it succeeds. I'll see what Travis-CI has to say about that: > https://travis-ci.org/smcv/dbus-mirror/builds/108266544 The answer is "don't apply this yet, there are still -Wpointer-sign issues".
(In reply to Simon McVittie from comment #75) > Please stop putting "Reviewed-by" on git commits before I've reviewed them! > A reader should be able to interpret Reviewed-by to mean "was actually > reviewed by", not "I hope it will be reviewed by". Where do you have seen this ? None of the three patches in this bug have a reviewed-by: line.
Created attachment 121645 [details] [review] _dbus_read_socket_with_unix_fds: make n_fds unsigned This makes it consistent with _dbus_message_loader_get_unix_fds().
Created attachment 121646 [details] [review] Consistently use socklen_t for getsockname, getsockopt etc. This fixes signedness mismatch warnings on platforms where socklen_t is unsigned, notably Linux (where it's an unsigned int). We still use int for the fallback case where the platform does not define socklen_t, because that was the traditional (pre-POSIX) type: for details see NOTES in Linux accept(2), <http://manpages.debian.org/cgi-bin/man.cgi?query=accept&sektion=2>.
Created attachment 121647 [details] [review] string_squash_nonprintable: correct signedness mismatch
Created attachment 121648 [details] [review] AppArmor: do not mix dbus_bool_t with int libdbus uses dbus_bool_t for booleans; that type is unsigned 32-bit. However, libapparmor uses int, which is signed, leading to -Wpointer-sign warnings when we pass a dbus_bool_t * where an int * was expected. This file is Linux-specific, and all Linux platforms have 32-bit int and an in-memory representation of the integers 0 and 1 that is independent of signedness, so the previous code was harmless in practice.
The additional patches I've attached here seem to fix all the remaining -Wpointer-sign. They should go in before Attachment #120145 [details].
(In reply to Ralf Habacker from comment #77) > Where do you have seen this ? None of the three patches in this bug have a > reviewed-by: line. In your 93069-gcc-5.2-warnings branch on ssh://people.freedesktop.org/~rhabacker/dbus
This is all cleanup rather than bug fixing: setting version to git master.
Created attachment 121654 [details] [review] Refactored cmake part dealing with compiler warnings to use warnings identifier (update 1)
Comment on attachment 121654 [details] [review] Refactored cmake part dealing with compiler warnings to use warnings identifier (update 1) Review of attachment 121654 [details] [review]: ----------------------------------------------------------------- ::: cmake/CMakeLists.txt @@ +189,5 @@ > + endif() > + > + # see https://msdn.microsoft.com/en-us/library/z78503e6.aspx > + # 14018 not documented > + set(WARNINGS "14018") According to https://msdn.microsoft.com/en-us/library/thxezb7y.aspx, /w14018 means "set warning #4018 to be a level-1 warning". 4018 seems to be basically -Wsign-compare, https://msdn.microsoft.com/en-us/library/y92ktdf2.aspx This might mean your macro should use /w1xxxx, /wdxxxx and /wexxxx for non-fatal, silenced and fatal, respectively? @@ +195,5 @@ > + # 4101 'identifier' : unreferenced local variable > + # 4127 conditional expression is constant > + # 4244 'argument' : conversion from 'type1' to 'type2', possible loss of data > + set(WARNINGS_DISABLED "4090 4101 4127 4244") > + # 4002 not documented Looks like this is "too many parameters for a macro": https://msdn.microsoft.com/en-us/library/y37zb304%28v=vs.71%29.aspx
Comment on attachment 121645 [details] [review] _dbus_read_socket_with_unix_fds: make n_fds unsigned Review of attachment 121645 [details] [review]: ----------------------------------------------------------------- looks good
Comment on attachment 121646 [details] [review] Consistently use socklen_t for getsockname, getsockopt etc. Review of attachment 121646 [details] [review]: ----------------------------------------------------------------- looks good
Comment on attachment 121647 [details] [review] string_squash_nonprintable: correct signedness mismatch Review of attachment 121647 [details] [review]: ----------------------------------------------------------------- looks good
Comment on attachment 121648 [details] [review] AppArmor: do not mix dbus_bool_t with int Review of attachment 121648 [details] [review]: ----------------------------------------------------------------- looks good
(In reply to Simon McVittie from comment #86) > Comment on attachment 121654 [details] [review] [review] > Refactored cmake part dealing with compiler warnings to use warnings > identifier (update 1) > > Review of attachment 121654 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: cmake/CMakeLists.txt > @@ +189,5 @@ > > + endif() > > + > > + # see https://msdn.microsoft.com/en-us/library/z78503e6.aspx > > + # 14018 not documented > > + set(WARNINGS "14018") > > According to https://msdn.microsoft.com/en-us/library/thxezb7y.aspx, /w14018 > means "set warning #4018 to be a level-1 warning". 4018 seems to be > basically -Wsign-compare, > https://msdn.microsoft.com/en-us/library/y92ktdf2.aspx > > This might mean your macro should use /w1xxxx, /wdxxxx and /wexxxx for > non-fatal, silenced and fatal, respectively? I'm going to check tomorrow, if we can use /w1xxxx unconditional or if it is better to stay with /wxxxx > @@ +195,5 @@ > > + # 4101 'identifier' : unreferenced local variable > > + # 4127 conditional expression is constant > > + # 4244 'argument' : conversion from 'type1' to 'type2', possible loss of data > > + set(WARNINGS_DISABLED "4090 4101 4127 4244") > > + # 4002 not documented > > Looks like this is "too many parameters for a macro": > https://msdn.microsoft.com/en-us/library/y37zb304%28v=vs.71%29.aspx sure, I will update the patch.
Comment on attachment 121645 [details] [review] _dbus_read_socket_with_unix_fds: make n_fds unsigned committed to master
Comment on attachment 121646 [details] [review] Consistently use socklen_t for getsockname, getsockopt etc. committed to master
Comment on attachment 121647 [details] [review] string_squash_nonprintable: correct signedness mismatch committed to master
Comment on attachment 121648 [details] [review] AppArmor: do not mix dbus_bool_t with int committed to master
Created attachment 121660 [details] [review] Refactored cmake part dealing with compiler warnings to use warnings identifier (update 2) - add missing warnings doc - use warning level 1, which is the default
(In reply to Simon McVittie from comment #76) > (In reply to Simon McVittie from comment #74) > > Yes if it succeeds. I'll see what Travis-CI has to say about that: > > https://travis-ci.org/smcv/dbus-mirror/builds/108266544 > > The answer is "don't apply this yet, there are still -Wpointer-sign issues". works now, see https://travis-ci.org/smcv/dbus-mirror/builds/108272409. Any more issues ?
Comment on attachment 121660 [details] [review] Refactored cmake part dealing with compiler warnings to use warnings identifier (update 2) Review of attachment 121660 [details] [review]: ----------------------------------------------------------------- Looks good, thanks. ::: cmake/CMakeLists.txt @@ +193,5 @@ > + set(WARNINGS "4018") > + # 4090 'operation' : different 'modifier' qualifiers > + # 4101 'identifier' : unreferenced local variable > + # 4127 conditional expression is constant > + # 4244 'argument' : conversion from 'type1' to 'type2', possible loss of data Apply this without changes first, but it would be interesting to see what warnings you get if you move these to be non-fatal warnings, particularly 4090 and 4244.
(In reply to Ralf Habacker from comment #97) > > The answer is "don't apply this yet, there are still -Wpointer-sign issues". > > works now, see https://travis-ci.org/smcv/dbus-mirror/builds/108272409. Yes, I think this should be good to go now. Thanks for working on this. I'll follow up with any further patches that are needed if my various test configurations (which are similar but not identical between travis-ci and my laptop) pick up any other -Wpointer-sign.
Comment on attachment 120145 [details] [review] Enable -Wpointer-sign warnings for autotools and cmake. committed to master
Comment on attachment 121660 [details] [review] Refactored cmake part dealing with compiler warnings to use warnings identifier (update 2) committed to master
Comment on attachment 120144 [details] [review] Keep cmake gcc builds in sync with autotools warnings. committed to master
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.