linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kalle Valo <kvalo@kernel.org>
To: Jeff Johnson <quic_jjohnson@quicinc.com>
Cc: <ath11k@lists.infradead.org>, <linux-wireless@vger.kernel.org>
Subject: Re: [PATCH 1/2] ath11k: mac: fix too long line
Date: Fri, 06 May 2022 12:04:42 +0300	[thread overview]
Message-ID: <87y1zf821h.fsf@kernel.org> (raw)
In-Reply-To: <1615688c-fb7d-261a-7b01-fe47c74b9597@quicinc.com> (Jeff Johnson's message of "Wed, 4 May 2022 09:30:35 -0700")

Jeff Johnson <quic_jjohnson@quicinc.com> writes:

> On 5/3/2022 10:58 PM, Kalle Valo wrote:
>> Jeff Johnson <quic_jjohnson@quicinc.com> writes:
> pe>> is there a reason to not declare deflink here?
>>> then its scope of definition would equal the scope of usage
>>
>> In ath10k and ath11k I have tried to avoid that and instead declare all
>> variables in the beginning of the function, this is to keep the code
>> simple. Of course there are few cases where a variable is declared in
>> the middle of the function, but that's just sloppy review on my part. I
>> feel that it's better to refactor the function into smaller functions
>> than start declaring variables in the middle of functions. Does that
>> make sense?
>>
>
> This is really an academic question.
>
> In the larger kernel community I'm seeing a push to reduce the scope
> of identifiers in some cases:
> 1) declaring loop control variables within the actual loop control operation
> 2) prohibiting the access to list iterators outside the list iteration loop

Yeah, those are definitely good examples when to limit scope.

And of course this isn't a hard rule, in some cases we have to make
exceptions. For example ath11k_mac_op_bss_info_changed() and
ath11k_mac_op_sta_state() are long functions but trying to split them to
smaller functions most likely makes them harder to read. So for these
functions having variables declared within if blocks would sound like a
good idea, I think it would make the functions more readable.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

  reply	other threads:[~2022-05-06  9:04 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-03  6:04 [PATCH 1/2] ath11k: mac: fix too long line Kalle Valo
2022-05-03  6:04 ` [PATCH 2/2] ath10k: mac: fix too long lines Kalle Valo
2022-05-03 15:22   ` Jeff Johnson
2022-05-03 15:17 ` [PATCH 1/2] ath11k: mac: fix too long line Jeff Johnson
2022-05-04  5:58   ` Kalle Valo
2022-05-04 16:30     ` Jeff Johnson
2022-05-06  9:04       ` Kalle Valo [this message]
2022-05-06  6:17 ` Kalle Valo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87y1zf821h.fsf@kernel.org \
    --to=kvalo@kernel.org \
    --cc=ath11k@lists.infradead.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=quic_jjohnson@quicinc.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).