On Fri, Feb 5, 2016 at 5:18 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>> Introduce align_atom_parser() which will parse an 'align' atom and
>> store the required alignment position and width in the 'used_atom'
>> structure for further usage in populate_value().
>>
>> Since this patch removes the last usage of match_atom_name(), remove
>> the function from ref-filter.c.
>>
>> Signed-off-by: Karthik Nayak <Karthik.188@xxxxxxxxx>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -55,6 +61,37 @@ static align_type parse_align_position(const char *s)
>> +static void align_atom_parser(struct used_atom *atom, const char *arg)
>> +{
>> + struct align *align = &atom->u.align;
>> + struct strbuf **s, **to_free;
>> + unsigned int width = ~0U;
>> +
>> + if (!arg)
>> + die(_("expected format: %%(align:<width>,<position>)"));
>> + s = to_free = strbuf_split_str_omit_term(arg, ',', 0);
>> +
>> + align->position = ALIGN_LEFT;
>> +
>> + while (*s) {
>> + int position;
>> + arg = s[0]->buf;
>
> It's confusing to see 'arg' being re-used here for a different
> purpose, and leads the reader to wonder if this is done because the
> s[0]->buf might be needed outside the loop (when, in fact, it isn't).
> It would be better to declare a new variable here in the scope of the
> 'while' loop to hold this value.
>
> (I might have named the result of the strbuf split 'tokens' or even
> short-and-sweet 'v' -- for vector -- and then used 's' for the name of
> the new variable here in the 'while' loop, but these name suggestions
> aren't particularly important; it is important to declare a new
> variable here -- whatever you name it -- rather than re-using 'arg'.)
>
You're right, that is indeed confusing, I should stop reusing variables
and trying to micromanage.
I also like the naming scheme you suggested, so will stick to that.
Thanks.
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html