Re: [PATCH 1/6] staging: android: binder: Remove some funny && usage

Jeremy Fitzhardinge
2009-06-17T15:28:49Z

On 06/17/09 07:37, Daniel Walker wrote:
> I agree it's reasonable in some cases.. The reason I changed this is
> because at first glance I didn't know what those lines were suppose to
> do. The equals signs all bleed together combined with the length of the
> statement makes it not match other similar usage. The if statement just
> makes the whole thing explicit.
>

I definitely see your point, but the if() statement variant has the
downside of only conditionally assigning the variable, and requiring it
to be initialized separately. I have a general code-cleanup rule to
convert:

foo = false;
if (something_is_true())
foo = true;

to

foo = something_is_true();

Maybe a bit of reformatting and some tactical use of parens would help?

wait_for_proc_work = (!thread->transaction_stack&& list_empty(&thread->todo));

(I'm not normally a fan of NULL-as-false, but it reads OK here.)

> Not to mention this code is a mess, very dense, and has little or no
> comments. Anything that can be done to make the code more clear, seem
> like a cleanup to me.
>

No argument from me. Not to mention that I have no idea from reading
the code what the whole subsystem is for; "Android IPC Subsystem"
doesn't tell me much, other than a gnawing feeling about having yet
another IPC subsystem to deal with.

> As for using "bool" , AFAIK that's only part of C++ ..
>

No, it is also C99, and becoming widely used in the kernel.

J