Re: [PATCH 3/7] powerpc/sstep: Fix W=1 compile warning

2020-09-11 Thread Christophe Leroy 




Le 11/09/2020 à 07:59, Cédric Le Goater a écrit :

On 9/11/20 7:38 AM, Christophe Leroy wrote:



Le 10/09/2020 à 23:02, Cédric Le Goater a écrit :

../arch/powerpc/lib/sstep.c: In function ‘mlsd_8lsd_ea’:
../arch/powerpc/lib/sstep.c:225:3: error: suggest braces around empty body in 
an ‘if’ statement [-Werror=empty-body]
     ; /* Invalid form. Should already be checked for by caller! */
     ^


A small sentence explaining how this is fixed would be welcome, so that you 
don't need to read the code the know what the commit does to fix the warning. 
Also the subject should be more explicit.





Cc: Jordan Niethe 
Signed-off-by: Cédric Le Goater 
---
   arch/powerpc/lib/sstep.c | 3 ++-
   1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index caee8cc77e19..14572af16e55 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -221,8 +221,9 @@ static nokprobe_inline unsigned long mlsd_8lsd_ea(unsigned 
int instr,
   ; /* Leave ea as is */
   else if (prefix_r && !ra)
   ea += regs->nip;
-    else if (prefix_r && ra)
+    else if (prefix_r && ra) {
   ; /* Invalid form. Should already be checked for by caller! */
+    }


You can't do that. Now checkpatch will complain that you don't have braces on 
all legs of the if/else dance.


Should we fix checkpatch ?


Why not, not then fix 
https://www.kernel.org/doc/html/latest/process/coding-style.html#placing-braces-and-spaces 
first :)


Christophe

Re: [PATCH 3/7] powerpc/sstep: Fix W=1 compile warning

2020-09-11 Thread Cédric Le Goater 
On 9/11/20 7:38 AM, Christophe Leroy wrote:
> 
> 
> Le 10/09/2020 à 23:02, Cédric Le Goater a écrit :
>> ../arch/powerpc/lib/sstep.c: In function ‘mlsd_8lsd_ea’:
>> ../arch/powerpc/lib/sstep.c:225:3: error: suggest braces around empty body 
>> in an ‘if’ statement [-Werror=empty-body]
>>     ; /* Invalid form. Should already be checked for by caller! */
>>     ^
> 
> A small sentence explaining how this is fixed would be welcome, so that you 
> don't need to read the code the know what the commit does to fix the warning. 
> Also the subject should be more explicit.
> 
> 
> 
>>
>> Cc: Jordan Niethe 
>> Signed-off-by: Cédric Le Goater 
>> ---
>>   arch/powerpc/lib/sstep.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
>> index caee8cc77e19..14572af16e55 100644
>> --- a/arch/powerpc/lib/sstep.c
>> +++ b/arch/powerpc/lib/sstep.c
>> @@ -221,8 +221,9 @@ static nokprobe_inline unsigned long 
>> mlsd_8lsd_ea(unsigned int instr,
>>   ; /* Leave ea as is */
>>   else if (prefix_r && !ra)
>>   ea += regs->nip;
>> -    else if (prefix_r && ra)
>> +    else if (prefix_r && ra) {
>>   ; /* Invalid form. Should already be checked for by caller! */
>> +    }
> 
> You can't do that. Now checkpatch will complain that you don't have braces on 
> all legs of the if/else dance.

Should we fix checkpatch ?

> I think the last 'else if' should simply be removed entirely as it does 
> nothing. Eventually, just leave the comment, something like:
> 
> /* (prefix_r && ra) is Invalid form. Should already be checked for by 
> caller! */
> 
> And if (prefix_r && ra) is not possible, then the previous if should just be 
> 'if (prefx_r)'

Indeed. I will rework that one.

Thanks,

C. 


> Christophe
> 
> 
>>     return ea;
>>   }
>>

Re: [PATCH 3/7] powerpc/sstep: Fix W=1 compile warning

2020-09-10 Thread Christophe Leroy 




Le 10/09/2020 à 23:02, Cédric Le Goater a écrit :

../arch/powerpc/lib/sstep.c: In function ‘mlsd_8lsd_ea’:
../arch/powerpc/lib/sstep.c:225:3: error: suggest braces around empty body in 
an ‘if’ statement [-Werror=empty-body]
; /* Invalid form. Should already be checked for by caller! */
^


A small sentence explaining how this is fixed would be welcome, so that 
you don't need to read the code the know what the commit does to fix the 
warning. Also the subject should be more explicit.






Cc: Jordan Niethe 
Signed-off-by: Cédric Le Goater 
---
  arch/powerpc/lib/sstep.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index caee8cc77e19..14572af16e55 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -221,8 +221,9 @@ static nokprobe_inline unsigned long mlsd_8lsd_ea(unsigned 
int instr,
; /* Leave ea as is */
else if (prefix_r && !ra)
ea += regs->nip;
-   else if (prefix_r && ra)
+   else if (prefix_r && ra) {
; /* Invalid form. Should already be checked for by caller! */
+   }


You can't do that. Now checkpatch will complain that you don't have 
braces on all legs of the if/else dance.


I think the last 'else if' should simply be removed entirely as it does 
nothing. Eventually, just leave the comment, something like:


	/* (prefix_r && ra) is Invalid form. Should already be checked for by 
caller! */


And if (prefix_r && ra) is not possible, then the previous if should 
just be 'if (prefx_r)'


Christophe


  
  	return ea;

  }