Re: [PATCH v2 5/6] grep: remove regflags from the public grep_opt API
From: Junio C Hamano <hidden>
Date: 2017-06-30 17:20:18
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted hunk
@@ -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 ;-).