Re: [PATCH v2 11/11] ref-filter: introduce objectname_atom_parser()

On Fri, Dec 25, 2015 at 11:39 PM, Eric Sunshine [off-list ref] wrote:

On Fri, Dec 25, 2015 at 8:44 AM, Karthik Nayak [off-list ref] wrote:

quoted

On Fri, Dec 18, 2015 at 11:54 AM, Eric Sunshine [off-list ref] wrote:

quoted

On Wed, Dec 16, 2015 at 10:30 AM, Karthik Nayak [off-list ref] wrote:

quoted

+static void objectname_atom_parser(struct used_atom *atom)
+{
+       const char * buf;
+
+       if (match_atom_name(atom->str, "objectname", &buf))
+               atom->u.objectname = O_FULL;
+       if (!buf)
+               return;

make_atom_name("objectname") will return true only for "objectname" or
"objectname:", and will return false for anything else, such as
"objectnamely" or "schmorf". Furthermore, the only way
objectname_atom_parser() can be called is when %(objectname) or
%(objectname:...) is seen, thus match_atom_name() *must* return true
here, which means the above conditional is misleading, suggesting that
it could somehow return false.

And, if match_atom_name() did return false here, then that indicates a
programming error: objectname_atom_parser() somehow got called for
something other than %(objectname) or %(objectname:...). This implies
that the code should instead be structured like this:

    if (!match_atom_name(..., "objectname", &buf)
        die("BUG: parsing non-'objectname'")
    if (!buf)
        atom->u.objectname = O_FULL;
    else if (!strcmp(buf, "short"))
        atom->u.objectname = O_SHORT;
    else
        die(_("unrecognized %%(objectname) argument: %s"), buf);

However, this can be simplified further by recognizing that, following
this patch series, match_atom_name() is *only* called by these new
parse functions[1], which means that, as a convenience,
match_atom_name() itself could become a void rather than boolean
function and die() if the expected atom name is not found. Thus, the
code would become:

    match_atom_name(...);
    if (!buf)
        ...
    else if (!strcmp(...))
        ...
    ...

By the way, the above commentary isn't specific to this patch and
%(objectname), but is in fact also relevant for all of the preceding
patches which introduce parse functions calling match_atom_name().

Ah! Thats some good observation, makes sense, match_atom_name()
is only called after the atom name, so making it return a indication of
success or failure doesn't make sense.

I think this change would go nicely with the introduction of 'enum atom_type'
which you mentioned[0] in the previous series.

I'm not sure to which change you refer as going nicely with the 'enum
atom_type'.

The observation above is about misleading logic in this series which
makes the code confusing. At the very least, the present series should
clean up the logic to reflect the first example above.

Changing match_atom_name() from boolean to void, as in the second
example above, is a nice little cleanup, but less important and
needn't be in this series (though it would just be one minor patch at
the end of the series).

I was referring to the second part as you just mentioned, But I could
at it to the
end of the series, I have restructured things as you mentioned in the example
though. Sorry for the confusion :)

-- 
Regards,
Karthik Nayak