[PATCH] Staging: qlge: Remove parenthesis around single condition

[PATCH] Staging: qlge: Remove parenthesis around single condition

[Sumitra Sharma]

At line #354 checkpatch.pl:
CHECK: Unnecessary parentheses around 'i == 0x00000114'
CHECK: Unnecessary parentheses around 'i == 0x00000118'
CHECK: Unnecessary parenthesis around 'i == 0x00000140'
CHECK: Unnecessary parentheses around 'i == 0x0000013c'

Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
---
 drivers/staging/qlge/qlge_dbg.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
index 66d28358342f..b190a2993033 100644
--- a/drivers/staging/qlge/qlge_dbg.c
+++ b/drivers/staging/qlge/qlge_dbg.c
@@ -351,10 +351,10 @@ static int qlge_get_xgmac_regs(struct qlge_adapter *qdev, u32 *buf,
 		/* We're reading 400 xgmac registers, but we filter out
 		 * several locations that are non-responsive to reads.
 		 */
-		if ((i == 0x00000114) ||
-		    (i == 0x00000118) ||
-			(i == 0x0000013c) ||
-			(i == 0x00000140) ||
+		if (i == 0x00000114 ||
+		    i == 0x00000118 ||
+			i == 0x0000013c ||
+			i == 0x00000140 ||
 			(i > 0x00000150 && i < 0x000001fc) ||
 			(i > 0x00000278 && i < 0x000002a0) ||
 			(i > 0x000002c0 && i < 0x000002cf) ||
-- 
2.25.1

Re: [PATCH] Staging: qlge: Remove parenthesis around single condition

[Dan Carpenter]

On Sat, Mar 11, 2023 at 06:04:09AM -0800, Sumitra Sharma wrote:
> At line #354 checkpatch.pl:
> CHECK: Unnecessary parentheses around 'i == 0x00000114'
> CHECK: Unnecessary parentheses around 'i == 0x00000118'
> CHECK: Unnecessary parenthesis around 'i == 0x00000140'
> CHECK: Unnecessary parentheses around 'i == 0x0000013c'
> 

Greg likes the extra parentheses so don't bother sending these sorts of
patches to staging.

> Signed-off-by: Sumitra Sharma <sumitraartsy@gmail.com>
> ---
>  drivers/staging/qlge/qlge_dbg.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/qlge/qlge_dbg.c b/drivers/staging/qlge/qlge_dbg.c
> index 66d28358342f..b190a2993033 100644
> --- a/drivers/staging/qlge/qlge_dbg.c
> +++ b/drivers/staging/qlge/qlge_dbg.c
> @@ -351,10 +351,10 @@ static int qlge_get_xgmac_regs(struct qlge_adapter *qdev, u32 *buf,
>  		/* We're reading 400 xgmac registers, but we filter out
>  		 * several locations that are non-responsive to reads.
>  		 */
> -		if ((i == 0x00000114) ||
> -		    (i == 0x00000118) ||
> -			(i == 0x0000013c) ||
> -			(i == 0x00000140) ||
> +		if (i == 0x00000114 ||
> +		    i == 0x00000118 ||
> +			i == 0x0000013c ||
> +			i == 0x00000140 ||

The weirder thing about this code is the indenting.  It should be:
[tab][tab][space][space][space][space](i ==.

regards,
dan carpenter

Re: [PATCH] Staging: qlge: Remove parenthesis around single condition

[Sumitra Sharma]

Hi Dan,

Your suggestion for correcting the indentation to
"[tab][tab][space][space][space][space](i ==." conflicts with the 
statement "Outside of comments, documentation and except in Kconfig, 
spaces are never used for indentation" written in 
https://elixir.bootlin.com/linux/latest/source/Documentation/process/coding-style.rst

However, If you still recommend to correct the indentation in the manner
"[tab][tab][space][space][space][space](i ==." Should I create a
patch for the same? 

Regards,

Sumitra

Re: [PATCH] Staging: qlge: Remove parenthesis around single condition

[Julia Lawall]

On Sat, 11 Mar 2023, Sumitra Sharma wrote:

> Hi Dan,
>
> Your suggestion for correcting the indentation to
> "[tab][tab][space][space][space][space](i ==." conflicts with the
> statement "Outside of comments, documentation and except in Kconfig,
> spaces are never used for indentation" written in
> https://elixir.bootlin.com/linux/latest/source/Documentation/process/coding-style.rst

You can use spaces at the end of an indentation if it allows lining up
things with a similar purpose.

Look around at other examples of kernel code, outside of staging, to see
what others have done.

julia

Re: [PATCH] Staging: qlge: Remove parenthesis around single condition

[Dan Carpenter]

On Sat, Mar 11, 2023 at 06:43:18AM -0800, Sumitra Sharma wrote:
> Hi Dan,
> 
> Your suggestion for correcting the indentation to
> "[tab][tab][space][space][space][space](i ==." conflicts with the 
> statement "Outside of comments, documentation and except in Kconfig, 
> spaces are never used for indentation" written in 
> https://elixir.bootlin.com/linux/latest/source/Documentation/process/coding-style.rst

Huh...  That documentation is very wrong.  Maybe you are not the only
person who has read that and been confused.  Not only do we use spaces,
but checkpatch will complain if you don't use spaces to align function
parameters.

Aligning conditions is not universal but it's generally prefered. There
isn't a checkpatch warning for misaligned conditions, but I think that's
because it's much trickier to parse conditions correctly.  Aligning
conditions is much more subtle.

What this means is that maybe you should consult with your mentor and
try to fix the documentation.  Maybe say something to the effect of "You
can use spaces for micro alignments to function parameters and
conditions in an if statement".

> 
> However, If you still recommend to correct the indentation in the manner
> "[tab][tab][space][space][space][space](i ==." Should I create a
> patch for the same? 

I'm not going to tell you what to do with your life.  :P  But if you
send that patch then we will merge it.

regards,
dan carpenter

Re: [PATCH] Staging: qlge: Remove parenthesis around single condition

[Fabio M. De Francesco]

On sabato 11 marzo 2023 15:43:18 CET Sumitra Sharma wrote:
> Hi Dan,
> 
> Your suggestion for correcting the indentation to
> "[tab][tab][space][space][space][space](i ==." conflicts with the
> statement "Outside of comments, documentation and except in Kconfig,
> spaces are never used for indentation" written in
> https://elixir.bootlin.com/linux/latest/source/Documentation/process/coding-style.rst
>

I just saw that you use to read .rst files. Attention!
These are source files from which human readable documentation is made.

They may contain directives which don't show a large part of the content you 
are interested in, which is only included when you run "make html", "make pdf" 
and similar commands.

Obviously, I'm talking about something that is _not_ related to your patch or 
this thread. I'm just concerned that candidates won't be able to find the 
information they're looking for and thus miss out on important information 
that Ira and I have asked candidates to study (if they're interested in 
applying to our project). 

In any case, the study of a certain number of pages of the Kernel 
documentation is always necessary, whatever project one wishes to undertake.

To better understand what I'm talking about, take the Highmem documentation as 
an example and compare the .rst file and the .html file. You will understand 
why I am strongly encouraging you and all other applicants not to use 
elixir.bootlin.com for study.

Please compare the content (or at least the number of sections and lines) of 
following .rst file at

https://elixir.bootlin.com/linux/v6.3-rc1/source/Documentation/mm/highmem.rst

with the human readable counterpart at 

https://docs.kernel.org/mm/highmem.html

Can you see how many information you may miss by reading an .rst file without 
any knowledge of its syntax?

Thanks,

Fabio

PS: I don't know why you went to the documentation source files. I see no 
reason other than the need to work on patches for the source document.

I'd like to invite you to patch the style guide that Dan suggested to you in 
this same thread.

But I'm not asking you to spend time on that patch during this contribution 
period because you still have a lot to do before the period expires.

If you want, you could take care of that patch after the contribution period 
has ended and while you are waiting for the outcome of the selection. If so, 
study the syntax of the .rst format carefully.

In the "real" kernel (I mean anywhere outside of drivers/staging), you might 
not always get the custom help of the same kind you can count on here. 
However, those who intend to go further, regardless of the current project, 
sooner or later will have to face the world outside :-)

>
> However, If you still recommend to correct the indentation in the manner
> "[tab][tab][space][space][space][space](i ==." Should I create a
> patch for the same?
> 
> Regards,
> 
> Sumitra

Re: [PATCH] Staging: qlge: Remove parenthesis around single condition

[Sumitra Sharma]

Hi Fabio,

Thank you for the insights. I went to the .rst files because they are directly linked in the first-patch document. I also noticed the difference between a ".rst" file and its counter human readable ".html" file. You were right that many information is being missed when anyone will read the .rst instead of .html/.pdf. I would like to suggest that the links that redirect to the .rst source files in the first-patch document must be changed to the links that redirect to there corresponding human readable format. Let me know if I could do it under the [KERNEL NEWBIEs ACCESS]. 

Apart from this I will be happy to patch the style guide after this
contribution period.

Regards,

Sumitra

Re: [PATCH] Staging: qlge: Remove parenthesis around single condition

[Fabio M. De Francesco]

On domenica 12 marzo 2023 08:17:00 CET Sumitra Sharma wrote:
> Hi Fabio,
> 
> Thank you for the insights.

Hi Sumitra,

Please pay attention to the suggestions that Julia kindly provides to you.

A moment ago I read these other words from her to you: "Sorry to press the 
point, but you should be doing it now, not promising to do it in the future.".

She is sorry to press because of her special attitudes. I've been helped 
uncountable times by Julia who, despite her major role and commitments, is so 
kind to spend time here in a Sunday morning. 

I think that it's you who should be sorry for pushing her that way... 

You are doing it again and again: in your last message you did not provide the 
necessary context. You are asking me to recall which "insights" you are 
talking about. I know for sure that here there is someone who read hundreds of 
emails per day.

> I went to the .rst files because they are directly
> linked in the first-patch document. I also noticed the difference between a
> ".rst" file and its counter human readable ".html" file. You were right that
> many information is being missed when anyone will read the .rst instead of
> .html/.pdf.

Don't summarize my own words in order to make me understand what question you 
are answering. Just put my question/objection/comment inline while replying.

Most mentors here read several tens of emails per day. You should not expect 
them to skim the whole thread. Just reply inline, so they find all the 
necessary context in one place and so there is no need to traverse the thread 
to understand what you are referring to. 

You have been told so several times. You risk that someone starts to ignore 
your messages in order to save time for other applicants.

> I would like to suggest that the links that redirect to the .rst
> source files in the first-patch document must be changed to the links that
> redirect to there corresponding human readable format. Let me know if I 
could
> do it under the [KERNEL NEWBIEs ACCESS].

This is a great idea and I fully agree with you.
Please ask Alison to grant access to change that documentation (it's not 
something I can do myself).

> Apart from this I will be happy to patch the style guide after this
> contribution period.

Best applicants go on with further contributions after the end of these 
period. This fact has been observed several times in the other rounds.
I'd like to see more people who are seriously willing to be part of this 
Community, regardless of the Outreachy project's deadlines. 

> Regards,
> 
> Sumitra

Thanks,

Fabio

P.S.: I probably went a bit too far in the first part of this email. I am 
sorry about it. However, please demonstrate that you finally got it, not with 
further promises but with actual changes in the way you communicate :-)