Hi Rene,
On 2026-05-21T13:22:19+0200, Rene Kita wrote:
> On Thu, May 21, 2026 at 06:20:04PM +0800, Kevin J. McCarthy wrote:
> > On Thu, May 21, 2026 at 11:24:38AM +0200, Alejandro Colomar via Mutt-dev
> > wrote:
> > > >
> > > > pager.c | 69 ++++++++++++++++++++++++++++++++++++---------------------
> > > > 1 file changed, 44 insertions(+), 25 deletions(-)
> >
> > Hi Alex,
> >
> > Thanks for the review.
> >
> > I should have mentioned that this patch is basically stitching together two
> > parts of the existing code. Both of them are old, and so are... to say it
> > politely... "compact".
> >
> > The search chunk was relocated into a function resolve_search().
> >
> > The newline removal/re-add was copied from a different function in the
> > pager code, match_body_patterns().
> >
> > > > diff --git a/pager.c b/pager.c
> > > > index 92fa152a..75221643 100644
> > > > --- a/pager.c
> > > > +++ b/pager.c
> > > > @@ -1268,6 +1268,47 @@ bail:
> > > > return rc;
> > > > }
> > > >
> > > > +static void resolve_search(struct line_t *lineInfo, int n, char *fmt,
> > > > + regex_t *SearchRE)
> > > > +{
> > > > + size_t fmtlen;
> > > > + int has_nl = 0, offset = 0;
> > > > + regmatch_t pmatch[1];
> > > > +
> > > > + /* don't consider line endings part of the buffer
> > > > + * for regex matching */
> > > > + if ((fmtlen = mutt_strlen(fmt)) > 0 && fmt[fmtlen - 1] == '\n')
> > >
> > > I would very much rewrite the above in two lines. I don't see a good
> > > reason to embed the assignment in the conditional (and that often leads
> > > to bugs due to accidents). I suggest:
> > >
> > > fmtlen = mutt_strlen(fmt);
> > > if (fmtlen > 0 && fmt[fmtlen - 1] == '\n')
> >
> > Sure I'll make this change. Again, in my defense, this is verbatim
> > pulled from match_body_patterns(). So although it's compact, I knew it
> > was working.
>
> I'd prefer the compact version, but that is just me.
Apart from the possibility of bugs, there's also the fact that our
brains often consider the 'if' line as part of the branch while reading,
and thus when thinking about the main algorithm implemented in the
function, we may ignore the condition in our brains, as it seems part of
a less important branch. However, 'fmtlen' is also used in the main
path of this function, so hiding it in the branch makes the code less
understandable, as one needs to consider branches for understanding the
main path.
Ideally, most functions could be understood even if we removed most
branches, as they often exist only for handling corner cases.
An exception is simple error conditionals, such as
if (foo(...) == -1) {
...
}
The reason is that since the 'if' line is simple (mostly just the
function call), and it's such a common idiom that we are used to seeing,
it doesn't distract much (the distraction of having a dummy variable in
the main path would be worse).
> I just wanted to
> speak up to say that such discussions are not helpful, IMHO.
I'd like to friendly disagree. Carefully written code tends to be
safer. These details might seem unimportant, but they add up. I don't
intend that we fix all the code base, but at least when we add new code,
I mention these little things, just in case anyone finds them useful.
For example, the cast I reported is fully redundant, but it doesn't do
anything wrong. We might not care about one cast which is known to not
hurt. But if we were to ignore each superfluous cast, we might end up
with thousands of superfluous casts, and whenever some code needs to be
audited later, seeing thousands of casts will certainly hurt.
Have a lovely day!
Alex
--
<https://www.alejandro-colomar.es>
signature.asc
Description: PGP signature
