linux-kernel - Re: [PATCH v4 3/5] staging: rtl8192e: fix coding style errors (macros in parentheses)

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] [day] [month] [year] [list]

Message-ID: <55076025.3060109@gmail.com>
Date:	Mon, 16 Mar 2015 23:58:45 +0100
From:	Mateusz Kulikowski <mateusz.kulikowski@...il.com>
To:	Joe Perches <joe@...ches.com>
CC:	gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 3/5] staging: rtl8192e: fix coding style errors (macros
 in parentheses)

On 16.03.2015 05:51, Joe Perches wrote:
> On Sun, 2015-03-15 at 21:39 +0100, Mateusz Kulikowski wrote:
>> Fix checkpatch.pl errors 'Macros with complex values should be enclosed in parentheses'.
> []
>> diff --git a/drivers/staging/rtl8192e/rtl819x_HT.h b/drivers/staging/rtl8192e/rtl819x_HT.h
> []
>> @@ -78,7 +78,7 @@ enum chnl_op {
>>  };
>>  
>>  #define CHHLOP_IN_PROGRESS(_pHTInfo)	\
>> -		((_pHTInfo)->ChnlOp > CHNLOP_NONE) ? true : false
>> +		(((_pHTInfo)->ChnlOp > CHNLOP_NONE) ? true : false)
> 
> This would perhaps be better without the ternary as
> the compiler might not optimize the ternary away
> 
> #define CHHLOP_IN_PROGRESS(_pHTInfo)				\
> 	((_pHTInfo)->ChnOp > CHNLOP_NONE)
> 
> This would also probably be better as a static inline
> not a macro, but as this is never used it's actually
> better just to remove it.

Thanks for the hints, done in v5 (other macros were also harmed)

> 
>> @@ -385,8 +385,8 @@ extern u8 MCS_FILTER_1SS[16];
>>  #define	LEGACY_WIRELESS_MODE	IEEE_MODE_MASK
>>  
>>  #define CURRENT_RATE(WirelessMode, LegacyRate, HTRate)	\
>> -			((WirelessMode & (LEGACY_WIRELESS_MODE)) != 0) ? \
>> -			(LegacyRate) : (PICK_RATE(LegacyRate, HTRate))
>> +			(((WirelessMode & (LEGACY_WIRELESS_MODE)) != 0) ? \
>> +			(LegacyRate) : (PICK_RATE(LegacyRate, HTRate)))
> 
> This would be better with the various macro arguments
> parenthesized (especially WirelessMode)
> 
> #define CURRENT_RATE(WirelessMode, LegacyRate, HTRate)		\
> 	(((WirelessMode) & LEGACY_WIRELESS_MODE)  ? 		\
> 	(LegacyRate) : PICK_RATE((LegacyRate), HTRate))
> 
> As this is used exactly once, it's probably better
> expanded in that one place and the macro removed.
> 
> That's true for PICK_RATE as well.

Will replace with static function (in .c file) - code that uses this macro is nested enough

Regards
Mateusz

--
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/