Re: [PATCH 5/5] tracing, MIPS: Fix set_graph_function of function graph tracer

[wu zhangjin <wuzhangjin@gmail.com>]

On Thu, Jan 20, 2011 at 10:04 PM, Steven Rostedt <srostedt@redhat.com> wrote:
[...]
>> > +
>> > +   insns = (in_kernel_space(self_ra)) ? 2 : (MCOUNT_OFFSET_INSNS + 1);
>>
>>     Unneeded parens.
>
> agreed
>
>>
>> > +   trace.func = self_ra - (MCOUNT_INSN_SIZE * insns);
>>
>>     Here too.
>
> I like the parens here. Yes, it is basic math precedence, but it stands
> out to a reviewer who has their brain more focused on correct code than
> thinking about which op has precedence.
>
> Reviewing code that has:
>
>        trace.func = self_ra - MCOUNT_INSN_SIZE * insns;
>
> And as I my mother language reads left to right, my thought process
> goes: subtract MCOUNT_INSN_SIZE from self_ra and then times insns... oh
> wait, times goes first; stop reset, restart... subtract MCOUNT_INSN_SIZE
> times insns from self_ra.  That stop reset and restart of the thought
> process breaks the train of thought and could waste a lot more time than
> just the moment it happened. All this is avoided by the parenthesis that
> automatically trigger the brain to think those go first.
>
> I say, leave these in.

Yeah, agree ;-)

parens have become one important part of my coding style for it have
saved me from being confused by the problems like you mentioned above.

This is similar to using "a=++i" or "i++; a=i;", the latter is longer
but clearer, although If we put them together, the former is also
clear, but If we encounter "a=++i;" and "a=i++;" at the same time, we
may have a headache ;-)

Will resend it asap with your feedback.

Thanks,
Wu Zhangjin

>
> -- Steve
>
>
>