Re: /bzr/squid3/trunk/ r12194: Small optimization in CommOpener statistic accounting.

On Tue, Jul 3, 2012 at 12:34 AM, Amos Jeffries <squid3_at_treenet.co.nz> wrote:
> On 03.07.2012 03:30, Francesco Chemolli wrote:
>>
>>
>> - if (conn_->getPeer())
>> - conn_->getPeer()->stats.conn_open++;
>> + if (peer *peer=(conn_->getPeer()))
>> + ++peer->stats.conn_open;
>>
>> lookupLocalAddress();
>
>
>
> Two points:
>
> 1) assignment in the if() needs to be double-bracketed around the whole =
> operator expression, not just the RHS:
> if ((peer *peer=conn_->getPeer()))

gcc accepts it, but sure.

> 2) This is the type of pre-increment operator use which I am a bit
> uncomfortable with.
>
> It is hard to tell when skimming the code at speed whether that should be
> read as:
>
> (++peer)->stats.conn_open;
> or
> ++(conn_->getPeer()->stats.conn_open);
>
>
> IMHO we should consistently use bracketing as above to clarify in situations
> like this where there is any complex location syntax.

It is the latter, but I had the some doubt so I double-checked. I
agree however that this may be not as readable as I'd like.
Ok for the bracketing, I'll review the cases that snuck in and correct them.

-- 
    /kinkie