Re: [PATCH v2 5/6] grep: remove regflags from the public grep_opt API
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 30, 2017, 17:20 UTC
- Message-ID
- <xmqqwp7ts88k.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170629222222.4694-6-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 21 quoted lines
> @@ -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 ;-).