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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 17 Jun 2009 08:28:26 -0700
From:	Jeremy Fitzhardinge <jeremy@...p.org>
To:	Daniel Walker <dwalker@...o99.com>
CC:	Greg Kroah-Hartman <greg@...ah.com>,
	Brian Swetland <swetland@...gle.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/6] staging: android: binder: Remove some funny &&	usage

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/