Re: [PATCH 1/2 v3] [libpq] rework sigpipe-handling macros

On Mon, Jul 20, 2009 at 3:14 AM, Jeremy Kerr<jk(at)ozlabs(dot)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