linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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>
Subject: Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().
Date: Sat, 5 Dec 2020 21:57:25 +0100	[thread overview]
Message-ID: <CAMj1kXFZ1_05O=s37n7pJbFxRm_-hAVFHKqc6OtALfBMVZVYnQ@mail.gmail.com> (raw)
In-Reply-To: <3161fc13d69c388b1f51f59c6ecea48dcd0a7856.camel@HansenPartnership.com>

On Sat, 5 Dec 2020 at 21:24, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
>
> On Sat, 2020-12-05 at 20:36 +0100, Ard Biesheuvel wrote:
> > On Fri, 4 Dec 2020 at 19:02, James Bottomley
> > <James.Bottomley@hansenpartnership.com> wrote:
> > > On Fri, 2020-12-04 at 18:07 +0100, Ard Biesheuvel wrote:
> > > > On Fri, 4 Dec 2020 at 18:06, <laniel_francis@privacyrequired.com>
> > > > wrote:
> > > > > From: Francis Laniel <laniel_francis@privacyrequired.com>
> > > > >
> > > > > The two functions indicates if a string begins with a given
> > > > > prefix. The only difference is that strstarts() returns a bool
> > > > > while str_has_prefix() returns the length of the prefix if the
> > > > > string begins with it or 0 otherwise.
> > > > >
> > > >
> > > > Why?
> > >
> > > I think I can answer that.  If the conversion were done properly
> > > (which it's not) you could get rid of the double strings in the
> > > code which are error prone if you update one and forget
> > > another.  This gives a good example: 3d739c1f6156 ("tracing: Use
> > > the return of str_has_prefix() to remove open coded numbers"). so
> > > in your code you'd replace things like
> > >
> > >     if (strstarts(option, "rgb")) {
> > >         option += strlen("rgb");
> > >         ...
> > >
> > > with
> > >
> > >     len = str_has_prefix(option, "rgb");
> > >     if (len) {
> > >         option += len
> > >         ...
> > >
> > > Obviously you also have cases where strstart is used as a boolean
> > > with no need to know the length ... I think there's no value to
> > > converting those.
> > >
> >
> > This will lead to worse code being generated. strlen() is evaluated
> > at build time by the compiler if the argument is a string literal, so
> > your 'before' version gets turned into 'option += 3', whereas the
> > latter needs to use a runtime variable.
>
> str_has_prefix() is an always_inline function so it should be build
> time evaluated as well.  I think most compilers see len as being a
> constant and unchanged, so elide the variable.  This means the code
> generated should be the same.
>

Fair enough. I wasn't aware str_has_prefix() was __always_inline.

> > 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.

  reply	other threads:[~2020-12-05 20:58 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-04 17:03 [RFC PATCH v1 00/12] " 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 [this message]
2020-12-05 21:15             ` James Bottomley
2020-12-05 21:20               ` Ard Biesheuvel
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='CAMj1kXFZ1_05O=s37n7pJbFxRm_-hAVFHKqc6OtALfBMVZVYnQ@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 \
    --subject='Re: [RFC PATCH v1 07/12] efi: Replace strstarts() by str_has_prefix().' \
    /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

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).