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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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
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
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
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
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
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 :-)