Re: [PATCH v3 00/15] ref-filter: use parsing functions
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Jan 7, 2016, 20:36 UTC
- Message-ID
- <CAPig+cSJ-6v2x4CY7h9OZQwSR4y9dXGnhxUv2Ykt997vx6gcZg@mail.gmail.com>
- In-Reply-To
- <xmqqvb7544de.fsf@gitster.mtv.corp.google.com>
On Thu, Jan 7, 2016 at 1:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 19 quoted lines
> If we really want to avoid having separate subject_atom_parser() and
> body_atom_parser(), they can be folded into the same function and it
> becomes necessary to switch on atom->name like you did in the code
> being discussed in the quoted part above. For that, as Eric said,
> skipping '*' manually would not be a big deal, as that should not
> happen so often in the code _anyway_. It is not a good idea to
> switch on atom->name inside contents_atom_parser() like you did.
> You are better off having separate {subject,body}_atom_parser()
> functions.
>
> For one thing, you are not reusing or sharing any code by squishing
> these three functions into one. A conceptually larger problem is
> that you are adding two extra !strcmp() calls to figure out the
> caller _already_ knows (notice I said this is "conceptual", this is
> not about performance). parse_ref_filter_atom() knows that it is a
> "%(subject)" or "%(subject:...)" atom, but because you throw away
> that information and call contents_atom_parser() by saying that it
> is one of the contents, subject or body, the called function has to
> redo strcmp in order to figure it out itself.Thanks for spelling this all out. I had come to the same conclusion yesterday after posting my message but never got back to the computer long enough to compose a proper email explaining the issue. I fully agree that this would be a better design.