diff mbox series

powerpc: replace ternary operator with min()

Message ID 4ebda26c.346.18404df6852.Coremail.wangkailong@jari.cn
State New
Headers
Series powerpc: replace ternary operator with min() |

Commit Message

KaiLong Wang Oct. 23, 2022, 12:44 p.m. UTC 
  Fix the following coccicheck warning:

arch/powerpc/xmon/xmon.c:2987: WARNING opportunity for min()
arch/powerpc/xmon/xmon.c:2583: WARNING opportunity for min()

Signed-off-by: KaiLong Wang <wangkailong@jari.cn>
---
 arch/powerpc/xmon/xmon.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Russell Currey Oct. 24, 2022, 4:33 a.m. UTC | #1 
On Sun, 2022-10-23 at 20:44 +0800, KaiLong Wang wrote:
> Fix the following coccicheck warning:
> 
> arch/powerpc/xmon/xmon.c:2987: WARNING opportunity for min()
> arch/powerpc/xmon/xmon.c:2583: WARNING opportunity for min()
> 
> Signed-off-by: KaiLong Wang <wangkailong@jari.cn>

Hello,

This fails to compile on some platforms/compilers since n is a long and
16 is an int, expanding to:

r = __builtin_choose_expr(
	((!!(sizeof((typeof(n) *)1 == (typeof(16) *)1))) &&
	 ((sizeof(int) ==
	   sizeof(*(8 ? ((void *)((long)(n)*0l)) : (int *)8))) &&
	  (sizeof(int) ==
	   sizeof(*(8 ? ((void *)((long)(16) * 0l)) :
			(int *)8))))),
	((n) < (16) ? (n) : (16)), ({
		typeof(n) __UNIQUE_ID___x0 = (n);
		typeof(16) __UNIQUE_ID___y1 = (16);
		((__UNIQUE_ID___x0) < (__UNIQUE_ID___y1) ?
			 (__UNIQUE_ID___x0) :
			 (__UNIQUE_ID___y1));
	}));

Here's the full build failure as found by snowpatch:
https://github.com/ruscur/linux-ci/actions/runs/3308880562/jobs/5461579048#step:4:89

You should use min_t(long, n, 16) instead.

- Russell

> ---
>  arch/powerpc/xmon/xmon.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
> index f51c882bf902..a7751cd2cc9d 100644
> --- a/arch/powerpc/xmon/xmon.c
> +++ b/arch/powerpc/xmon/xmon.c
> @@ -2580,7 +2580,7 @@ static void xmon_rawdump (unsigned long adrs,
> long ndump)
>         unsigned char temp[16];
>  
>         for (n = ndump; n > 0;) {
> -               r = n < 16? n: 16;
> +               r = min(n, 16);
>                 nr = mread(adrs, temp, r);
>                 adrs += nr;
>                 for (m = 0; m < r; ++m) {
> @@ -2984,7 +2984,7 @@ prdump(unsigned long adrs, long ndump)
>         for (n = ndump; n > 0;) {
>                 printf(REG, adrs);
>                 putchar(' ');
> -               r = n < 16? n: 16;
> +               r = min(n, 16);
>                 nr = mread(adrs, temp, r);
>                 adrs += nr;
>                 for (m = 0; m < r; ++m) {
kernel test robot Oct. 26, 2022, 12:07 p.m. UTC | #2 
Hi KaiLong,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on linus/master v6.1-rc2 next-20221026]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/KaiLong-Wang/powerpc-replace-ternary-operator-with-min/20221023-204906
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
patch link:    https://lore.kernel.org/r/4ebda26c.346.18404df6852.Coremail.wangkailong%40jari.cn
patch subject: [PATCH] powerpc: replace ternary operator with min()
config: powerpc-ppc64e_defconfig (attached as .config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 791a7ae1ba3efd6bca96338e10ffde557ba83920)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install powerpc cross compiling tool for clang build
        # apt-get install binutils-powerpc-linux-gnu
        # https://github.com/intel-lab-lkp/linux/commit/51fa624eb9fa01ea67de462263913ab61a68cbc5
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review KaiLong-Wang/powerpc-replace-ternary-operator-with-min/20221023-204906
        git checkout 51fa624eb9fa01ea67de462263913ab61a68cbc5
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=powerpc SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> arch/powerpc/xmon/xmon.c:2583:7: error: comparison of distinct pointer types ('typeof (n) *' (aka 'long *') and 'typeof (16) *' (aka 'int *')) [-Werror,-Wcompare-distinct-pointer-types]
                   r = min(n, 16);
                       ^~~~~~~~~~
   include/linux/minmax.h:45:19: note: expanded from macro 'min'
   #define min(x, y)       __careful_cmp(x, y, <)
                           ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:36:24: note: expanded from macro '__careful_cmp'
           __builtin_choose_expr(__safe_cmp(x, y), \
                                 ^~~~~~~~~~~~~~~~
   include/linux/minmax.h:26:4: note: expanded from macro '__safe_cmp'
                   (__typecheck(x, y) && __no_side_effects(x, y))
                    ^~~~~~~~~~~~~~~~~
   include/linux/minmax.h:20:28: note: expanded from macro '__typecheck'
           (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
                      ~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~
   arch/powerpc/xmon/xmon.c:2987:7: error: comparison of distinct pointer types ('typeof (n) *' (aka 'long *') and 'typeof (16) *' (aka 'int *')) [-Werror,-Wcompare-distinct-pointer-types]
                   r = min(n, 16);
                       ^~~~~~~~~~
   include/linux/minmax.h:45:19: note: expanded from macro 'min'
   #define min(x, y)       __careful_cmp(x, y, <)
                           ^~~~~~~~~~~~~~~~~~~~~~
   include/linux/minmax.h:36:24: note: expanded from macro '__careful_cmp'
           __builtin_choose_expr(__safe_cmp(x, y), \
                                 ^~~~~~~~~~~~~~~~
   include/linux/minmax.h:26:4: note: expanded from macro '__safe_cmp'
                   (__typecheck(x, y) && __no_side_effects(x, y))
                    ^~~~~~~~~~~~~~~~~
   include/linux/minmax.h:20:28: note: expanded from macro '__typecheck'
           (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1)))
                      ~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~
   2 errors generated.


vim +2583 arch/powerpc/xmon/xmon.c

  2576	
  2577	static void xmon_rawdump (unsigned long adrs, long ndump)
  2578	{
  2579		long n, m, r, nr;
  2580		unsigned char temp[16];
  2581	
  2582		for (n = ndump; n > 0;) {
> 2583			r = min(n, 16);
  2584			nr = mread(adrs, temp, r);
  2585			adrs += nr;
  2586			for (m = 0; m < r; ++m) {
  2587				if (m < nr)
  2588					printf("%.2x", temp[m]);
  2589				else
  2590					printf("%s", fault_chars[fault_type]);
  2591			}
  2592			n -= r;
  2593			if (nr < r)
  2594				break;
  2595		}
  2596		printf("\n");
  2597	}
  2598
Christophe Leroy Nov. 2, 2022, 9:23 a.m. UTC | #3 
Le 24/10/2022 à 06:33, Russell Currey a écrit :
> On Sun, 2022-10-23 at 20:44 +0800, KaiLong Wang wrote:
>> Fix the following coccicheck warning:
>>
>> arch/powerpc/xmon/xmon.c:2987: WARNING opportunity for min()
>> arch/powerpc/xmon/xmon.c:2583: WARNING opportunity for min()
>>
>> Signed-off-by: KaiLong Wang <wangkailong@jari.cn>
> 
> Hello,
> 
> This fails to compile on some platforms/compilers since n is a long and
> 16 is an int, expanding to:
> 
> r = __builtin_choose_expr(
> 	((!!(sizeof((typeof(n) *)1 == (typeof(16) *)1))) &&
> 	 ((sizeof(int) ==
> 	   sizeof(*(8 ? ((void *)((long)(n)*0l)) : (int *)8))) &&
> 	  (sizeof(int) ==
> 	   sizeof(*(8 ? ((void *)((long)(16) * 0l)) :
> 			(int *)8))))),
> 	((n) < (16) ? (n) : (16)), ({
> 		typeof(n) __UNIQUE_ID___x0 = (n);
> 		typeof(16) __UNIQUE_ID___y1 = (16);
> 		((__UNIQUE_ID___x0) < (__UNIQUE_ID___y1) ?
> 			 (__UNIQUE_ID___x0) :
> 			 (__UNIQUE_ID___y1));
> 	}));
> 
> Here's the full build failure as found by snowpatch:
> https://github.com/ruscur/linux-ci/actions/runs/3308880562/jobs/5461579048#step:4:89
> 
> You should use min_t(long, n, 16) instead.

Wouldn't it work with 16L instead of 16 :

	min(n, 16L)

Christophe

> 
> - Russell
> 
>> ---
>>   arch/powerpc/xmon/xmon.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
>> index f51c882bf902..a7751cd2cc9d 100644
>> --- a/arch/powerpc/xmon/xmon.c
>> +++ b/arch/powerpc/xmon/xmon.c
>> @@ -2580,7 +2580,7 @@ static void xmon_rawdump (unsigned long adrs,
>> long ndump)
>>          unsigned char temp[16];
>>   
>>          for (n = ndump; n > 0;) {
>> -               r = n < 16? n: 16;
>> +               r = min(n, 16);
>>                  nr = mread(adrs, temp, r);
>>                  adrs += nr;
>>                  for (m = 0; m < r; ++m) {
>> @@ -2984,7 +2984,7 @@ prdump(unsigned long adrs, long ndump)
>>          for (n = ndump; n > 0;) {
>>                  printf(REG, adrs);
>>                  putchar(' ');
>> -               r = n < 16? n: 16;
>> +               r = min(n, 16);
>>                  nr = mread(adrs, temp, r);
>>                  adrs += nr;
>>                  for (m = 0; m < r; ++m) {
>
diff mbox series

Patch

diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index f51c882bf902..a7751cd2cc9d 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -2580,7 +2580,7 @@  static void xmon_rawdump (unsigned long adrs, long ndump)
 	unsigned char temp[16];
 
 	for (n = ndump; n > 0;) {
-		r = n < 16? n: 16;
+		r = min(n, 16);
 		nr = mread(adrs, temp, r);
 		adrs += nr;
 		for (m = 0; m < r; ++m) {
@@ -2984,7 +2984,7 @@  prdump(unsigned long adrs, long ndump)
 	for (n = ndump; n > 0;) {
 		printf(REG, adrs);
 		putchar(' ');
-		r = n < 16? n: 16;
+		r = min(n, 16);
 		nr = mread(adrs, temp, r);
 		adrs += nr;
 		for (m = 0; m < r; ++m) {