On Mon, Jul 20, 2009 at 3:14 AM, Jeremy Kerr<j...@ozlabs.org> wrote: >> That code is not broken as it stands, and doesn't appear to really >> gain anything from the proposed change. Why should we risk any >> portability questions when the code isn't going to get either simpler >> or shorter? > > OK, after attempting a macro version of this, we end up with: > > #define DECLARE_SIGPIPE_INFO(var) struct sigpipe_info var; > > #define DISABLE_SIGPIPE(info, conn) \ > ((SIGPIPE_MASKED(conn)) ? 0 : \ > ((info)->got_epipe = false, \ > pg_block_sigpipe(&(info)->oldsigmask, > &(info)->sigpipe_pending)) < 0) > > - kinda ugly, uses the ?: and , operators to return the result of > pg_block_sigpipe. We could avoid the comma with a block though. > > If we want to keep the current 'failaction' style of macro: > > #define DISABLE_SIGPIPE(info, conn, failaction) \ > do { \ > if (!SIGPIPE_MASKED(conn)) { \ > (info)->got_epipe = false; \ > if (pg_block_sigpipe(&(info)->oldsigmask, \ > &(info)->sigpipe_pending)) < 0) { \ > failaction; \ > } \ > } \ > } while (0) > > We could ditch struct sigpipe info, but that means we need to declare > three separate vars in DECLARE_SIGPIPE_INFO() instead of the one, and it > doesn't clean up the macro code much. I'd rather reduce the amount of > stuff that we declare "behind the caller's back". > > Compared to the static-function version: > > static inline int disable_sigpipe(PGconn *conn, struct sigpipe_info > *info) > { > if (sigpipe_masked(conn)) > return 0; > > info->got_epipe = false; > return pq_block_sigpipe(&info->oldsigmask, &info->sigpipe_pending) < 0; > } > > Personally, I think the static functions are a lot cleaner, and don't > think we lose any portability from using these (since inline is #define- > ed out on compilers that don't support it). On non-inlining compilers, > we do gain an extra function call, but we're about to enter the kernel > anyway, so this will probably be lost in the noise (especially if we > save the sigpipe-masking syscalls). > > But in the end, it's not up to me - do you still prefer the macro > approach?
Since Tom seems to prefer the macro approach, and since the current code uses a macro, I think we should stick with doing it that way. Also, as some of Tom's comments above indicate, I don't think it's making anything any easier for anyone that you keep submitting this as two separate patches. It's one thing to submit a patch in pieces of it is very large or complex and especially if the pieces are independent, but that's not really the case here. Because we are now over a week into this CommitFest, we need to get a final, reviewable version of this patch as quickly as possible. So please make the requested changes and resubmit as soon as you can. Thanks, ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers