Hi,
On Tue, Aug 25, 2020 at 1:32 AM Hariom verma <hariom18599@xxxxxxxxx> wrote:
> On Mon, Aug 24, 2020 at 9:19 AM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> >
> > On Sun, Aug 23, 2020 at 8:56 PM Hariom verma <hariom18599@xxxxxxxxx> wrote:
> > > Recently, I sent a patch series "Improvements to ref-filter"[1]. A
> > > patch in this patch series introduced "sanitize" modifier to "subject"
> > > atom. i.e "%(subject:sanitize)".
> > >
> > > What if in the future we also want "%(contents:subject:sanitize)" to work?
> > > We can use this helper any number of times, whenever there is a need.
> > >
> > > Sorry, I missed saying this earlier. But I don't prefer duplicating
> > > the code here.
> >
> > Pushing back on a reviewer suggestion is fine. Explaining the reason
> > for your position -- as you do here -- helps reviewers understand why
> > you feel the way you do. My review suggestion about making it easier
> > to reason about the code while avoiding a brand new function, at the
> > cost of a minor amount of duplication, was made in the context of this
> > one-off case in which the function increased cognitive load and was
> > used just once (not knowing that you envisioned future callers). If
> > you expect the new function to be re-used by upcoming changes, then
> > that may be a good reason to keep it. Stating so in the commit message
> > will help reviewers see beyond the immediate patch or patch series.
>
> Yeah. I should have mentioned this in the commit message.
I agree.
> > Aside from a couple minor style violations[1,2], I don't particularly
> > oppose the helper function, though I have a quibble with the name
> > check_format_field(), which I don't find helpful, and which (at least
> > for me) increases the cognitive load. The increased cognitive load, I
> > think, comes not only from the function name not spelling out what the
> > function actually does, but also because the function is dual-purpose:
> > it's both checking that the argument matches a particular token
> > ("trailers", in this case) and extracting the sub-argument. Perhaps
> > naming it match_and_extract_subarg() or something similar would help,
> > though that's a mouthful.
>
> I will fix those violations.
> Also, "match_and_extract_subarg()" looks good to me.
I am not sure about the "subarg" part of the name. In the for-each-ref
doc, names inside %(...) are called "field names", and parts after ":"
are called "options". So it might be better to have "field_option"
instead of "subarg" in the name.
I think we could also get rid of the "match_and_" part of the
suggestion, in the same way as skip_prefix() is not called
match_and_skip_prefix(). Readers can just expect that if there is no
match the function will return 0.
So maybe "extract_field_option()".
> > But the observation about the function being dual-purpose (thus
> > potentially confusing) brings up other questions. For instance, is it
> > too special-purpose? If you foresee more callers in the future with
> > multiple-token arguments such as `%(content:subject:sanitize)`, should
> > the function provide more assistance by splitting out each of the
> > sub-arguments rather than stopping at the first? Taking that even
> > further, a generalized helper for "splitting" arguments like that
> > might be useful at the top-level of contents_atom_parser() too, rather
> > than only for specific arguments, such as "trailers". Of course, this
> > may all be way too ambitious for this little bug fix series or even
> > for whatever upcoming changes you're planning, thus not worth
> > pursuing.
>
> Splitting sub-arguments is done at "<atomname>_atom_parser()".
> If you mean pre-splitting every argument...
> something like: ['contents', 'subject', 'sanitize'] for
> `%(content:subject:sanitize)` in `contents_atom_parser()` ? I'm not
> able to see how it can be useful.
Yeah, it seems to me that such a splitting would require a complete
rewrite of the current code, so I am not sure it's an interesting way
forward for now. And anyway adding extract_field_option() goes in the
right direction of abstracting the parsing and making the code
simpler, more efficient and likely more correct.
> Sorry, If I got your concerned wrong.
>
> > As for the helper's implementation, I might have written it like this:
> >
> > static int check_format_field(...)
> > {
> > const char *opt
> > if (!strcmp(arg, field))
> > *option = NULL;
> > else if (skip_prefix(arg, field, opt) && *opt == ':')
> > *option = opt + 1;
> > else
> > return 0;
> > return 1;
> > }
> >
> > which is more compact and closer to what I suggested earlier for
> > avoiding the helper function in the first place. But, of course,
> > programming is quite subjective, and you may find your implementation
> > easier to reason about. Plus, your version has the benefit of being
> > slightly more optimal since it avoids an extra string scan, although
> > that probably is mostly immaterial considering that
> > contents_atom_parser() itself contains a long chain of potentially
> > sub-optimal strcmp() and skip_prefix() calls.
>
> "programming is quite subjective"
> Yeah, I couldn't agree more.
>
> The change you suggested looks good too. But I'm little inclined to my
> keeping my changes. I'm curious, what others have to say on this.
I also prefer a slightly more optimal one even if it's a bit less compact.
Thanks,
Christian.
