Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> @@ -169,6 +167,24 @@ void grep_init(struct grep_opt *opt, const char *prefix)
>
> static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
> {
> + /*
> + * When committing to the pattern type by setting the relevant
> + * fields in grep_opt it's generally not necessary to zero out
> + * the fields we're not choosing, since they won't have been
> + * set by anything. The extended_regexp_option field is the
> + * only exception to this.
> + *
> + * This is because in the process of parsing grep.patternType
> + * & grep.extendedRegexp we set opt->pattern_type_option and
> + * opt->extended_regexp_option, respectively. We then
> + * internally use opt->extended_regexp_option to see if we're
> + * compiling an ERE. It must be unset if that's not actually
> + * the case.
> + */
> + if (pattern_type != GREP_PATTERN_TYPE_ERE &&
> + opt->extended_regexp_option)
> + opt->extended_regexp_option = 0;
Good to have the reasoning in an in-code comment like the above.
But after reading these two paragraphs and then before reading the
three line code, a more natural embodiment in the code of the
commentary that came to my mind was
if (pattern_type != GREP_PATTERN_TYPE_ERE)
opt->extended_regexp_option = 0;
The end-result is the same as yours, of course, but I somehow found
it match the reasoning better.
Now, I wonder if this can further be tweaked to
opt->extended_regexp_option = (pattern_type == GREP_PATTERN_TYPE_ERE);
which might lead us in a direction to really unify the two related
fields extended_regexp_option and pattern_type_option.
Even if that were a good longer term direction to go in, it is
outside the scope of this step, of course. I am merely bringing it
up as an conversation item ;-).
