git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Masahiro Yamada <masahiroy@kernel.org>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] dir: remove unneeded local variables from match_pathname()
Date: Fri, 10 Feb 2023 13:51:50 -0800	[thread overview]
Message-ID: <xmqqfsbdgpe1.fsf@gitster.g> (raw)
In-Reply-To: <20230210045119.25190-1-masahiroy@kernel.org> (Masahiro Yamada's message of "Fri, 10 Feb 2023 13:51:19 +0900")

Masahiro Yamada <masahiroy@kernel.org> writes:

> The local variables are unneeded - you can simply advance the 'pathname'
> pointer.

It probably is somewhat subjective if it makes the resulting code
easier or harder to read with these extra variables, even though
"are unneeded" may technically be correct and the compilers may
produce identical binaries with or without the patch.

In the context of the original, this used to be in a loop where
elements of an array was matched against a constant pathname
variable, and it was necessary to use <name, namelen>, separate
variables, to point to the "remainder" of "pathname".  It would not
have made any sense not to use separate variables in that loop.

When the body of the loop was split into this helper function in
b5592632 (exclude: split pathname matching code into a separate
function, 2012-10-15), we could have removed these variables and
instead clobbered <pathname, pathlen>, but apparently we did not.  I
suspect that the original author found it easier to reason about the
behaviour of the function to keep the incoming parameter anchored at
the constant location, and use separate variables to point at the
tail part of the string that are to be worked on, which I tend to
disagree, but I do not have a strong preference.

Having said all that, I consider this to fall into "once the code is
written one way, it is not worth the patch noise to go and change it
to a different way." category.

Thanks.

      reply	other threads:[~2023-02-10 21:52 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-02-10  4:51 [PATCH] dir: remove unneeded local variables from match_pathname() Masahiro Yamada
2023-02-10 21:51 ` Junio C Hamano [this message]

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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=xmqqfsbdgpe1.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=masahiroy@kernel.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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).