fixes to src to prep for enabling additional compiler warnings #273

dgrove-oss · 2017-07-05T22:10:11Z

work in progress on cleaning up code so it can be compiled
with the same set of warning flags used on Darwin. This is an inital
pass through the C files in src to resolve warnings. Most warnings
are related to implicit size/sign conversions between integral types.

dgrove-oss · 2017-07-05T22:12:47Z

happy to adjust based on feedback. I've mainly gone with local fixes/casts to resolve the warnings and tried to avoid changing very much cross-platform code. This might not always have been the right thing to do...

MadCoder · 2017-07-14T01:46:42Z

src/event/event_epoll.c

@@ -89,12 +89,12 @@ _dispatch_muxnote_bucket(int ident)
 	return &_dispatch_sources[DSL_HASH((uint32_t)ident)];
 }
 #define _dispatch_unote_muxnote_bucket(du) \
-	_dispatch_muxnote_bucket(du._du->du_ident)
+	_dispatch_muxnote_bucket((int)du._du->du_ident)


I think _dispatch_muxnote_bucket() argument type should take a uint32_t instead.

MadCoder · 2017-07-14T01:47:18Z

src/event/event_epoll.c


 DISPATCH_ALWAYS_INLINE
 static inline dispatch_muxnote_t
 _dispatch_muxnote_find(struct dispatch_muxnote_bucket_s *dmb,
-		uint64_t ident, int16_t filter)
+		int ident, int16_t filter)


int seems the wrong type since it forces a cast at the call site, why int?

we need a cast somewhere on this call path because dmn_ident is an int (makes since since file descriptors are ints), but du_ident is an uint32_t.

I can push the cast down to the == compare instead of bubbling it up.

my worry is that signed stuff for a hash table is a bad idea. Moving to uint32_t would be fine fWIW. and the reason for uint32_t for du_identis because it's unsigned on Darwin because of kevent and it'll be hell if we change signedness of this field.

ok, I changed dmn_ident to be a uint32_t for the epoll backend.

MadCoder · 2017-07-14T01:48:02Z

src/event/event_epoll.c

 	int16_t filter = du._du->du_filter;
 	bool socket_listener = false;
 	sigset_t sigmask;

 	switch (filter) {
 	case EVFILT_SIGNAL:
-		if (!sigismember(&signals_with_unotes, du._du->du_ident)) {
+		if (!sigismember(&signals_with_unotes, (int)du._du->du_ident)) {


I would take a temporary int signo = (int)du._du->du_ident which would make it clearer in the first place and avoid that many ugly casts

MadCoder · 2017-07-14T01:48:30Z

src/event/event_epoll.c

@@ -343,32 +343,32 @@ _dispatch_event_merge_timer(dispatch_clock_t clock)
 }

 static void
-_dispatch_timeout_program(uint32_t tidx, uint64_t target, uint64_t leeway)
+_dispatch_timeout_program(uint32_t tidx, uint64_t target, DISPATCH_UNUSED uint64_t leeway)


80 columns (?) you need to wrap

MadCoder · 2017-07-14T01:49:49Z

src/init.c

@@ -614,13 +614,13 @@ _dispatch_bug(size_t line, long val)
 			_dispatch_build, (unsigned long)line, val);
 }

+#if HAVE_MACH


this seems wrong, are you sure it's never used on linux ?

the prototype in internal.h is already guarded this way, but agree that that seems wrong. This is currently only used in event_kevent.c (so could be guarded by DISPATCH_EVENT_BACKEND_KEVENT) but it is really intended to be a generic facility that can be used anywhere, so the better fix is probably to remove the ifdef of the prototype in internal.h

MadCoder · 2017-07-14T01:50:35Z

src/internal.h

@@ -466,7 +466,7 @@ void _dispatch_log(const char *msg, ...);
 		} \
 	} while (0)
 #else
-static inline void _dispatch_assert(long e, long line) {
+static inline void _dispatch_assert(long e, size_t line) {


can you fix the style while at it, it's unlike the rest of dispatch, it should be:

static inline void _dispatch_assert(long e, size_t line) {

thanks

MadCoder · 2017-07-14T01:50:42Z

src/internal.h

@@ -488,7 +488,7 @@ static inline void _dispatch_assert(long e, long line) {
 		} \
 	} while (0)
 #else
-static inline void _dispatch_assert_zero(long e, long line) {
+static inline void _dispatch_assert_zero(long e, size_t line) {


MadCoder · 2017-07-14T01:51:02Z

src/io.c

@@ -2167,7 +2167,7 @@ _dispatch_operation_advise(dispatch_operation_t op, size_t chunk_size)
 	op->advise_offset += advise.ra_count;
 #ifdef __linux__
 	_dispatch_io_syscall_switch(err,
-		readahead(op->fd_entry->fd, advise.ra_offset, advise.ra_count),
+								readahead(op->fd_entry->fd, advise.ra_offset, (size_t)advise.ra_count),


that indent seems wrong :)

MadCoder · 2017-07-14T01:51:29Z

src/queue.c

@@ -5330,6 +5330,7 @@ _dispatch_root_queue_push(dispatch_queue_t rq, dispatch_object_t dou,
 		return _dispatch_root_queue_push_override(rq, dou, qos);
 	}
 #endif
+	(void)qos;


please inside a #else instead

MadCoder · 2017-07-14T01:52:04Z

src/queue.c

@@ -6218,6 +6221,7 @@ DISPATCH_NORETURN
 static void
 _dispatch_deferred_items_cleanup(void *ctxt)
 {
+	(void)ctxt;


I think DISPATCH_INTERNAL_CRASH should ((void)(ac)) instead to avoid this

das · 2017-07-14T01:49:35Z

src/voucher.c

@@ -1550,12 +1550,14 @@ _voucher_create_accounting_voucher(voucher_t voucher)
 	return NULL;
 }

+#if __has_include(<mach/mach.h>)


I think this should be HAVE_MACH, that matches the guards of this type

das · 2017-07-14T01:56:56Z

src/data.c

@@ -129,6 +129,9 @@ static void
 _dispatch_data_destroy_buffer(const void* buffer, size_t size,
 		dispatch_queue_t queue, dispatch_block_t destructor)
 {
+#if !HAVE_MACH


would be nicer as an #else to the #if HAVE_MACH below

das · 2017-07-14T02:03:53Z

src/init.c

@@ -614,13 +614,13 @@ _dispatch_bug(size_t line, long val)
 			_dispatch_build, (unsigned long)line, val);
 }

+#if HAVE_MACH


the prototype in internal.h is already guarded this way, but agree that that seems wrong. This is currently only used in event_kevent.c (so could be guarded by DISPATCH_EVENT_BACKEND_KEVENT) but it is really intended to be a generic facility that can be used anywhere, so the better fix is probably to remove the ifdef of the prototype in internal.h

dgrove-oss · 2017-07-17T15:54:45Z

Thanks for the comments; I will revise/rebase and let you know when it is ready for second review (was out on vacation last week...)

dgrove-oss · 2017-07-18T20:35:45Z

updated to address comments, rebased to tip of master, & squashed. Ready for you to look at again.

das · 2017-07-20T22:46:43Z

src/event/event_epoll.c

 		}
 		sigemptyset(&sigmask);
-		sigaddset(&sigmask, du._du->du_ident);
+		sigaddset(&sigmask, (int)du._du->du_ident);


whoops, missed that one. fixed.

das

lgtm, thanks!

work in progress on cleaning up code so it can be compiled with the same set of warning flags used on Darwin. This is an initial pass through the C files in src to resolve warnings. Most warnings are related to implicit size/sign conversions between integral types and missing explicit prototypes for non-static functions.

das · 2017-07-24T18:36:45Z

@swift-ci please test

fixes to src to prep for enabling additional compiler warnings Signed-off-by: Daniel A. Steffen <dsteffen@apple.com>

dgrove-oss force-pushed the match-darwin-cflags-round1 branch from 2cd7124 to e6030f1 Compare July 6, 2017 13:11

dgrove-oss changed the title ~~fixes for compiler warnings~~ fixes to src to prep for enabling additional compiler warnings Jul 6, 2017

MadCoder reviewed Jul 14, 2017

View reviewed changes

das requested changes Jul 14, 2017

View reviewed changes

dgrove-oss force-pushed the match-darwin-cflags-round1 branch 2 times, most recently from 9d64682 to 6a3f928 Compare July 18, 2017 20:12

das reviewed Jul 20, 2017

View reviewed changes

das approved these changes Jul 20, 2017

View reviewed changes

dgrove-oss force-pushed the match-darwin-cflags-round1 branch from 6a3f928 to 03cd694 Compare July 21, 2017 00:06

das merged commit 698d085 into swiftlang:master Jul 24, 2017

das added the darwin pending label Jul 24, 2017

dgrove-oss deleted the match-darwin-cflags-round1 branch July 25, 2017 12:40

das added a commit that referenced this pull request Jul 31, 2017

Merge pull request #273 from dgrove-oss/match-darwin-cflags-round1

54651ca

fixes to src to prep for enabling additional compiler warnings Signed-off-by: Daniel A. Steffen <dsteffen@apple.com>

das mentioned this pull request Jul 31, 2017

Merge darwin/libdispatch-913.1.4 to master #289

Merged

das removed the darwin pending label Aug 1, 2017

fixes to src to prep for enabling additional compiler warnings #273

fixes to src to prep for enabling additional compiler warnings #273

Uh oh!

Conversation

dgrove-oss commented Jul 5, 2017

Uh oh!

dgrove-oss commented Jul 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgrove-oss commented Jul 17, 2017

Uh oh!

dgrove-oss commented Jul 18, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

das left a comment

Choose a reason for hiding this comment

Uh oh!

das commented Jul 24, 2017

Uh oh!

Uh oh!