From: Kalle Valo <kvalo@codeaurora.org>
To: Julia Lawall <julia.lawall@lip6.fr>
Cc: SF Markus Elfring <elfring@users.sourceforge.net>,
Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>,
libertas-dev@lists.infradead.org, linux-wireless@vger.kernel.org,
netdev@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
kernel-janitors@vger.kernel.org
Subject: Re: [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker()
Date: Thu, 21 Jan 2016 17:07:34 +0200 [thread overview]
Message-ID: <87si1rx96x.fsf@kamboji.qca.qualcomm.com> (raw)
In-Reply-To: <alpine.DEB.2.02.1601020914100.2063@localhost6.localdomain6> (Julia Lawall's message of "Sat, 2 Jan 2016 09:21:37 +0100 (CET)")
Julia Lawall <julia.lawall@lip6.fr> writes:
> On Sat, 2 Jan 2016, SF Markus Elfring wrote:
>
>> >> Move the jump label directly before the desired log statement
>> >> so that the variable "err" will not be checked once more
>> >> after it was determined that a function call failed.
>> >> Use the identifier "report_failure" instead of the label "err".
>> >
>> > Why?
>>
>> I suggest to reconsider the places with which such a jump label
>> is connected.
>>
>>
>> > The code was smart enough
>>
>> Which action should really be performed after a failure was detected
>> and handled a bit already?
>>
>> * Another condition check
>>
>> * Just additional error logging
>>
>>
>> > and you're making it uglier that it needs to be.
>>
>> I assume that a software development taste can evolve, can't it?
>
> So far, you have gotten several down votes for this kind of change, and no
> enthusiasm.
>
> Admittedly, this is a trivial case, because there are no local variables,
> but do you actually know the semantics in C of a jump into a block? And
> if you do know, do you think that this semantics is common knowledge? And
> do you really think that introducing poorly understandable code is really
> worth saving an if test of a single variable on a non-critical path?
>
> Most of the kernel code is not performance critical at the level of a
> single if test. So the goal should be for the code to be easy to
> understand and robust to change. The code that is performance critical,
> you should probably not touch, ever. The people who wrote it knew what
> was important and what was not.
Very well said! Only optimise something you can measure.
I'm dropping this patch.
--
Kalle Valo
next prev parent reply other threads:[~2016-01-21 15:07 UTC|newest]
Thread overview: 86+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <566ABCD9.1060404@users.sourceforge.net>
2016-01-01 18:21 ` [PATCH 0/2] net-ath9k_htc: Fine-tuning for two function implementations SF Markus Elfring
2016-01-01 18:23 ` [PATCH 1/2] net-ath9k_htc: Delete an unnecessary variable initialisation in ath9k_hif_usb_rx_stream() SF Markus Elfring
2016-01-01 19:14 ` Oleksij Rempel
2016-01-01 18:25 ` [PATCH 2/2] net-ath9k_htc: Replace a variable initialisation by an assignment in ath9k_htc_set_channel() SF Markus Elfring
2016-01-01 19:14 ` Oleksij Rempel
2016-04-08 1:40 ` Julian Calaby
2016-04-15 12:09 ` Kalle Valo
2016-04-15 14:34 ` Julian Calaby
2016-04-19 16:13 ` Kalle Valo
2016-01-01 19:26 ` [PATCH] net-brcmfmac: Delete an unnecessary variable initialisation in brcmf_sdio_download_firmware() SF Markus Elfring
2016-01-02 8:50 ` Arend van Spriel
2016-01-14 6:58 ` Kalle Valo
2016-01-01 20:27 ` [PATCH 0/3] net-iwlegacy: Fine-tuning for il_eeprom_init() SF Markus Elfring
2016-01-01 20:30 ` [PATCH 1/3] net-iwlegacy: Refactoring " SF Markus Elfring
2016-01-04 9:33 ` Stanislaw Gruszka
2016-01-01 20:31 ` [PATCH 2/3] net-iwlegacy: One check less in il_eeprom_init() after error detection SF Markus Elfring
2016-01-01 23:13 ` Sergei Shtylyov
2016-01-01 20:32 ` [PATCH 3/3] net-iwlegacy: Another refactoring for il_eeprom_init() SF Markus Elfring
2016-01-02 18:18 ` Souptick Joarder
2016-01-01 21:33 ` [PATCH] net-libertas: Better exception handling in if_spi_host_to_card_worker() SF Markus Elfring
2016-01-01 21:41 ` Julia Lawall
2016-01-01 23:14 ` Sergei Shtylyov
2016-01-02 8:09 ` SF Markus Elfring
2016-01-02 8:21 ` Julia Lawall
2016-01-02 9:08 ` SF Markus Elfring
2016-01-02 10:13 ` Arend van Spriel
2016-01-02 11:21 ` SF Markus Elfring
2016-01-03 9:36 ` Arend van Spriel
2016-01-03 12:13 ` SF Markus Elfring
2016-01-03 15:18 ` Rafał Miłecki
2016-01-04 10:05 ` Arend van Spriel
2016-01-04 11:18 ` Rafał Miłecki
2016-01-21 15:07 ` Kalle Valo [this message]
2016-01-02 14:40 ` [PATCH 0/3] net-rsi: Fine-tuning for two function implementations SF Markus Elfring
2016-01-02 14:43 ` [PATCH 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt() SF Markus Elfring
2016-01-02 15:12 ` net-rsi: Reconsider usage of variable "vap_id" " SF Markus Elfring
2016-01-02 16:32 ` [PATCH 1/3] rsi: Delete unnecessary variable initialisations " kbuild test robot
2016-01-02 18:27 ` [PATCH v2 " SF Markus Elfring
2016-01-04 9:28 ` [PATCH " Dan Carpenter
2016-01-04 9:38 ` Dan Carpenter
2016-01-04 10:44 ` SF Markus Elfring
2016-01-04 11:48 ` Dan Carpenter
2016-01-04 12:33 ` SF Markus Elfring
2016-01-04 23:54 ` Julian Calaby
2016-01-05 8:29 ` SF Markus Elfring
2016-01-05 9:47 ` Julian Calaby
2016-01-05 16:23 ` SF Markus Elfring
2016-01-04 13:17 ` [PATCH 1/3] " Bjørn Mork
2016-01-04 14:25 ` Dan Carpenter
2016-01-04 17:14 ` David Miller
2016-01-02 14:44 ` [PATCH 2/3] rsi: Delete unnecessary variable initialisations in rsi_send_data_pkt() SF Markus Elfring
2016-01-02 14:45 ` [PATCH 3/3] rsi: Replace variable initialisations by assignments " SF Markus Elfring
2016-01-02 15:07 ` Francois Romieu
2016-01-15 13:04 ` [PATCH v3 0/3] net-rsi: Fine-tuning for two function implementations SF Markus Elfring
2016-01-15 13:09 ` [PATCH v3 1/3] rsi: Delete unnecessary variable initialisations in rsi_send_mgmt_pkt() SF Markus Elfring
2016-01-15 13:10 ` [PATCH v3 2/3] rsi: Delete unnecessary variable initialisations in rsi_send_data_pkt() SF Markus Elfring
2016-01-15 13:12 ` [PATCH v3 3/3] rsi: Replace variable initialisations by assignments " SF Markus Elfring
2016-01-19 12:40 ` Dan Carpenter
2016-01-02 20:51 ` [PATCH 0/3] NFC-mei_phy: Fine-tuning for two function implementations SF Markus Elfring
2016-01-02 20:54 ` [PATCH 1/3] NFC-mei_phy: Refactoring for mei_nfc_connect() SF Markus Elfring
2016-01-02 23:41 ` Julian Calaby
2016-01-03 7:00 ` SF Markus Elfring
2016-01-03 7:29 ` Joe Perches
2016-01-02 20:55 ` [PATCH 2/3] NFC-mei_phy: Refactoring for mei_nfc_if_version() SF Markus Elfring
2016-01-02 20:56 ` [PATCH 3/3] NFC-mei_phy: Delete an unnecessary variable initialisation in mei_nfc_if_version() SF Markus Elfring
2016-08-20 16:43 ` [PATCH 0/3] hostap: Fine-tuning for a few functions SF Markus Elfring
2016-08-20 16:45 ` [PATCH 1/3] hostap: Use memdup_user() rather than duplicating its implementation SF Markus Elfring
2016-09-26 14:19 ` [1/3] " Kalle Valo
2016-08-20 16:46 ` [PATCH 2/3] hostap: Delete an unnecessary jump label in prism2_ioctl_priv_hostapd() SF Markus Elfring
2016-08-21 1:45 ` Julian Calaby
2016-09-26 15:06 ` [2/3] " Kalle Valo
[not found] ` <20160926150656.213D961568@smtp.codeaurora.org>
2016-09-26 16:03 ` SF Markus Elfring
2016-09-26 18:01 ` Kalle Valo
2016-09-26 18:06 ` SF Markus Elfring
2016-09-26 18:18 ` Joe Perches
2016-09-26 18:37 ` Kalle Valo
2016-09-26 18:43 ` Joe Perches
2016-08-20 16:48 ` [PATCH 3/3] hostap: Delete unnecessary initialisations for the variable "ret" SF Markus Elfring
2016-08-20 19:26 ` [PATCH 0/3] hostap: Fine-tuning for a few functions Arend van Spriel
2016-08-22 15:49 ` Kalle Valo
2016-08-22 16:18 ` Joe Perches
2016-08-22 18:17 ` [PATCH] checkpatch: See if modified files are marked obsolete in MAINTAINERS Joe Perches
2016-08-22 20:50 ` SF Markus Elfring
2016-08-22 20:56 ` Joe Perches
2016-08-23 7:26 ` SF Markus Elfring
2016-08-23 10:18 ` Julia Lawall
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=87si1rx96x.fsf@kamboji.qca.qualcomm.com \
--to=kvalo@codeaurora.org \
--cc=elfring@users.sourceforge.net \
--cc=julia.lawall@lip6.fr \
--cc=kernel-janitors@vger.kernel.org \
--cc=libertas-dev@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=sergei.shtylyov@cogentembedded.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).