Thanks. I see this patch was also installed in June so will close the bug
report.
I have a bone to pick with it, though. For bool expressions without side
effects, bitwise ops are logically equivalent to short-circuit ops. On modern
platforms where operands are trivial, short-circuit ops are often a tad bigger
and can have more branch-prediction overhead, which is a tiny bit worse for
performance. In the examples you the short-circuit ops all generate bigger
machine code on my platform (GCC 6.1 x86-64). So I'm mildly inclined to revert
the change and have pushed the attached. To some extent this is a style issue;
yes, there is a long tradition in C of using short-circuit ops for bool, but I
don't mind too much diverging slightly from the common style in order to make
the point.
For clarity this patch also uses c_isdigit (which uses a switch) instead of &&
though GCC seems to generate the same code either way.
From 02d761d5bcbdd7f55763d876715730df704df8c8 Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Thu, 18 Aug 2016 22:43:28 -0700
Subject: [PATCH] grep: prefer bitwise to short-circuit when shorter
* src/grep.c (skip_devices, initialize_unibyte_mask, fillbuf, main)
* src/kwsearch.c (Fexecute): Prefer bitwise to short-circuit ops
when they are logically equivalent and the bitwise ops generate
shorter code on GCC 6.1 x86-64.
* src/grep.c (get_nondigit_option, parse_grep_colors):
Use c_isdigit instead of spelling it out with a short-circuit op.
---
src/grep.c | 12 ++++++------
src/kwsearch.c | 4 ++--
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/grep.c b/src/grep.c
index a82da61..cc46919 100644
--- a/src/grep.c
+++ b/src/grep.c
@@ -560,7 +560,7 @@ static bool
skip_devices (bool command_line)
{
return (devices == SKIP_DEVICES
- || (devices == READ_COMMAND_LINE_DEVICES && !command_line));
+ || ((devices == READ_COMMAND_LINE_DEVICES) & !command_line));
}
/* Return if ST->st_size is defined. Assume the file is not a
@@ -642,7 +642,7 @@ initialize_unibyte_mask (void)
unsigned char mask = 0;
int ms1b = 1;
for (int i = 1; i <= UCHAR_MAX; i++)
- if (mbclen_cache[i] != 1 && ! (mask & i))
+ if ((mbclen_cache[i] != 1) & ! (mask & i))
{
while (ms1b * 2 <= i)
ms1b *= 2;
@@ -952,7 +952,7 @@ fillbuf (size_t save, struct stat const *st)
}
bufoffset += fillsize;
- if (fillsize == 0 || !skip_nuls || !all_zeros (readbuf, fillsize))
+ if (((fillsize == 0) | !skip_nuls) || !all_zeros (readbuf, fillsize))
break;
totalnl = add_count (totalnl, fillsize);
@@ -2152,7 +2152,7 @@ get_nondigit_option (int argc, char *const *argv, intmax_t *default_context)
{
opt = getopt_long (argc, (char **) argv, short_options,
long_options, NULL);
- if ( ! ('0' <= opt && opt <= '9'))
+ if (! c_isdigit (opt))
break;
if (prev_digit_optind != this_digit_optind || !was_digit)
@@ -2244,7 +2244,7 @@ parse_grep_colors (void)
}
else if (val == NULL)
q++; /* Accumulate name. */
- else if (*q == ';' || (*q >= '0' && *q <= '9'))
+ else if (*q == ';' || c_isdigit (*q))
q++; /* Accumulate val. Protect the terminal from being sent crap. */
else
return;
@@ -2702,7 +2702,7 @@ main (int argc, char **argv)
count_matches = false;
done_on_match = true;
}
- out_quiet = count_matches || done_on_match;
+ out_quiet = count_matches | done_on_match;
if (out_after < 0)
out_after = default_context;
diff --git a/src/kwsearch.c b/src/kwsearch.c
index d2afa40..09af4a2 100644
--- a/src/kwsearch.c
+++ b/src/kwsearch.c
@@ -94,7 +94,7 @@ Fexecute (char *buf, size_t size, size_t *match_size,
else
{
mb_check = MB_CUR_MAX > 1 && !using_utf8 ();
- longest = mb_check || start_ptr || match_words;
+ longest = mb_check | !!start_ptr | match_words;
}
for (mb_start = beg = start_ptr ? start_ptr : buf; beg <= buf + size; beg++)
@@ -124,7 +124,7 @@ Fexecute (char *buf, size_t size, size_t *match_size,
continue;
}
beg += offset;
- if (start_ptr && !match_words)
+ if (!!start_ptr & !match_words)
goto success_in_beg_and_len;
if (match_lines)
{
--
2.5.5
