On Thu, Dec 17, 2015 at 3:24 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Wed, Dec 16, 2015 at 10:29 AM, 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 "use_atom"
>> structure for further usage in 'populate_value()'.
>>
>> Helped-by: Ramsay Jones <ramsay@xxxxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Karthik Nayak <Karthik.188@xxxxxxxxx>
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -63,6 +69,49 @@ static void color_atom_parser(struct used_atom *atom)
>> +static align_type parse_align_position(const char *s)
>> +{
>> + if (!strcmp(s, "right"))
>> + return ALIGN_RIGHT;
>> + else if (!strcmp(s, "middle"))
>> + return ALIGN_MIDDLE;
>> + else if (!strcmp(s, "left"))
>> + return ALIGN_LEFT;
>> + return -1;
>> +}
>> +
>> +static void align_atom_parser(struct used_atom *atom)
>> +{
>> + struct align *align = &atom->u.align;
>> + const char *buf = NULL;
>> + struct strbuf **s, **to_free;
>> + int width = -1;
>> +
>> + match_atom_name(atom->str, "align", &buf);
>> + if (!buf)
>> + die(_("expected format: %%(align:<width>,<position>)"));
>> + s = to_free = strbuf_split_str_omit_term(buf, ',', 0);
>> +
>> + align->position = ALIGN_LEFT;
>> +
>> + while (*s) {
>> + int position;
>> +
>> + if (!strtoul_ui(s[0]->buf, 10, (unsigned int *)&width))
>
> This casting is pretty ugly. It probably would be cleaner to declare
> 'width' as 'unsigned int' and initialize it with ~0U than to do this
> ugly and potentially dangerous casting. Likewise, below where you
> check 'width < 0', you'd check instead for ~0U. However, such a change
> should not be part of the current patch, but rather as a separate
> preparatory patch.
>
Will do.
>> + ;
>> + else if ((position = parse_align_position(s[0]->buf)) > 0)
>
> Shouldn't this be '>=' rather than '>'?
>
> This likely would have been easier to spot if parse_align_position()
> had been factored out in its own patch, as suggested by my v1
> review[1], since the caller would have been trivially inspectable
> rather than having to keep track of both code movement and changes.
>
> [1]: http://article.gmane.org/gmane.comp.version-control.git/281916
>
Yes. it should. Will split this into three patches.
1. Creation of align_atom_parser().
2. Introduce parse_align_position().
3. make 'width' an unsigned int.
--
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