Subject: Re: libssh2 master 37624b6 sftp_attrsize: converted function to a macro

Re: libssh2 master 37624b6 sftp_attrsize: converted function to a macro

From: Alexander Lamaison <swish_at_lammy.co.uk>
Date: Wed, 28 Apr 2010 13:37:44 +0100

On 28 April 2010 07:38, Daniel Stenberg <daniel_at_haxx.se> wrote:
> On Wed, 28 Apr 2010, Alexander Lamaison wrote:
>
>>> sftp_attrsize: converted function to a macro
>>
>>> This way, the macro can evaluate a static number at compile time
>>> for two out of four uses, and it probably runs faster for the
>>> other two cases too.
>>
>> Oh please no.
>>
>> These kind of hand optimisations are almost always useless.  Was there any
>> other reason for the conversion except performance?  If not, was there a
>> benchmark to show this was a bottleneck and the macro fixed it?
>
> First, I disagree that these things "are almost always useless".

I would love to see evidence of their benefit.

> But my primary motivation for chaning this was that I really didn't like how
> the function took a struct pointer as argument and only checked/used the
> flag field within the function, and in 50% of the use cases it is a static
> flag known at compile-time.
>
> To me, this makes it A LOT clearer that sftp_attrsize() only checks the flag
> (and no other attr struct magic is involved) and returns a size based on
> that.

This is great. But can we have it as a function? Is there any reason
not to write:

/* sftp_attrsize
 * Size that attr with this flagset will occupy when turned into a bin struct
 */
static int
sftp_attrsize(unsigned long flags)
{
    return (4 + /* flags(4) */
        ((flags & LIBSSH2_SFTP_ATTR_SIZE)?8:0) +
        ((flags & LIBSSH2_SFTP_ATTR_UIDGID)?8:0) +
        ((flags & LIBSSH2_SFTP_ATTR_PERMISSIONS)?4:0) +
        ((flags & LIBSSH2_SFTP_ATTR_ACMODTIME)?8:0)); /* atime + mtime as u32 */
}

> It being a macro or function is less important then, but I just didn't
> think a full function was motivated once I changed the input argument - and
> a macro seemed simple and easy enough.

I don't think we should introduce any new macros to libssh2 unless
there is no other way to achieve the same effect (e.g. BLOCK_ADJUST).
They just aren't worth the reduction in safety, readability and
maintainability.

Alex
_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
Received on 2010-04-28