From: Ard Biesheuvel <ardb@kernel.org>
To: James Bottomley <James.Bottomley@hansenpartnership.com>
Cc: laniel_francis@privacyrequired.com,
linux-efi <linux-efi@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
Date: Sat, 5 Dec 2020 22:20:31 +0100 [thread overview]
Message-ID: <CAMj1kXHj0y9b+yGPDjyToFL6HYyyu23BuX3FMYmjGo5+6sgjUQ@mail.gmail.com> (raw)
In-Reply-To: <043040d9c092cedcab8bf88b0ec805616d3be44d.camel@HansenPartnership.com>
On Sat, 5 Dec 2020 at 22:15, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> [Rostedt added because this is all his fault]
> On Sat, 2020-12-05 at 21:57 +0100, Ard Biesheuvel wrote:
> > On Sat, 5 Dec 2020 at 21:24, James Bottomley
> > <James.Bottomley@hansenpartnership.com> wrote:
> [...]
> > > > So I don't object to using str_has_prefix() in new code in this
> > > > way, but I really don't see the point of touching existing code.
> > >
> > > That's your prerogative as a Maintainer ... I was just explaining
> > > what the original author had in mind when str_has_prefix() was
> > > created.
> > >
> >
> > Sure, I fully understand you are not the one proposing these changes.
> >
> > But if the pattern in question is so common, couldn't we go one step
> > further and define something like
> >
> > static inline const u8 *skip_prefix_or_null(const u8 *str, const u8
> > *prefix)
> > {
> > }
> >
> > which returns a pointer into the original string, or NULL if the
> > prefix is not present.
> >
> > The current patch as proposed has no benefit whatsoever, but even the
> > meaningful alternative you are proposing is not actually an
> > improvement, given that it is not self-explanatory from the name
> > 'str_has_prefix' what it returns, and so the code becomes more
> > difficult to understand.
>
> Ah, so this is the kernel maintainer's syndrome: you see an API which
> isn't quite right for your use case, so you update or change it. Then
> you see other use cases for it and suddenly to you it becomes the best
> thing since sliced bread and with a one ring to rule them all mentality
> you exhort everyone to use this new API everywhere. See this comment
> in the merge commit (495d714ad1400) which comes from the merge cover
> letter:
>
> > - Addition of str_has_prefix() and a few use cases. There
> > currently is a similar function strstart() that is used in a
> > few places, but only returns a bool and not a length. These
> > instances will be removed in the future to use
> > str_has_prefix() instead.
>
> Then you forget about it until someone else acts on your somewhat ill
> considered instruction and actually tries the replacement. Once
> someone takes up your cause, the API shows up in dozens of emails and
> the actual debate about whether or not this is such a good API really
> begins, with the poor person who picked it up caught in the crossfire.
>
> As maintainers we really should learn to put the cart before the horse.
>
I am not disagreeing with any of this, but I simply don't see a point
in merging patches that apparently result in the exact same machine
code to be generated, and don't substantially make the code itself any
better.
next prev parent reply other threads:[~2020-12-05 21:21 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-04 17:03 [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix() laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 01/12] arm: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 02/12] mips: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 03/12] crypto: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 04/12] device-mapper: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 05/12] renesas: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 06/12] omap: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 07/12] efi: " laniel_francis
2020-12-04 17:07 ` Ard Biesheuvel
2020-12-04 17:19 ` Francis Laniel
2020-12-04 18:02 ` James Bottomley
2020-12-05 19:08 ` Francis Laniel
2020-12-05 19:36 ` Ard Biesheuvel
2020-12-05 20:24 ` James Bottomley
2020-12-05 20:57 ` Ard Biesheuvel
2020-12-05 21:15 ` James Bottomley
2020-12-05 21:20 ` Ard Biesheuvel [this message]
2020-12-05 23:04 ` James Bottomley
2020-12-07 15:10 ` Steven Rostedt
2020-12-07 16:25 ` David Laight
2020-12-05 20:28 ` Rasmus Villemoes
2020-12-10 18:14 ` Arvind Sankar
2020-12-11 9:45 ` David Laight
2020-12-11 16:10 ` Arvind Sankar
2020-12-04 17:03 ` [RFC PATCH v1 08/12] ide: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 09/12] mips: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 10/12] module: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 11/12] musb: " laniel_francis
2020-12-04 17:03 ` [RFC PATCH v1 12/12] string.h: Remove strstarts() laniel_francis
2020-12-04 17:56 ` [RFC PATCH v1 00/12] Replace strstarts() by str_has_prefix() James Bottomley
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=CAMj1kXHj0y9b+yGPDjyToFL6HYyyu23BuX3FMYmjGo5+6sgjUQ@mail.gmail.com \
--to=ardb@kernel.org \
--cc=James.Bottomley@hansenpartnership.com \
--cc=laniel_francis@privacyrequired.com \
--cc=linux-efi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=rostedt@goodmis.org \
/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).