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