Re: [PATCH v2 00/10] port branch.c to use ref-filter's printing options

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Oct 8, 2015, 17:10 UTC
Message-ID: <vpq8u7dp9qr.fsf@grenoble-inp.fr>
In-Reply-To: <CAOLa=ZQvB_S2-nw8hOABt7aQJOWJXvfK1U2zurpnZmaAgJNnGA@mail.gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

Show 16 quoted lines

> On Thu, Oct 8, 2015 at 5:57 PM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> +                             unsigned int nobracket = 0;
>>> +
>>> +                             if (!strcmp(valp, ",nobracket"))
>>> +                                     nobracket = 1;
>>
>> The code to parse comma-separated values is different here and
>> elsewhere. I'd rather have the same code (possibly factored into a
>> helper function), both to get consistent behavior (you're not allowing
>> %(upstream:nobracket,track) for example, right?) and consistent code.
>>
>
> Speaking of comma-separated values, the only other place we use that is
> in the align atom. Also I find this very specific to get a function out of.
> Somehow I think this is the simplest way to go about this.

Well, most pieces of code start with one instance, then two, then more ;-). When the second instance starts being different from the first, it doesn't give a good example for the future third instance.

This particular piece of code is so important and I won't fight for a better factored one, but in general "there are only two instances" is a dubious argument to avoid refactoring.

Still, I find it weird to force the nobracket to be always at the same position.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/