On Thu, Dec 17, 2015 at 3:24 AM, Eric Sunshine <[email protected]> wrote: > On Wed, Dec 16, 2015 at 10:29 AM, Karthik Nayak <[email protected]> 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 <[email protected]> >> Signed-off-by: Karthik Nayak <[email protected]> >> --- >> 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
