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

On Thu, Dec 17, 2015 at 3:24 AM, Eric Sunshine [off-list ref] wrote:

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

quoted

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 [off-list ref]
Signed-off-by: Karthik Nayak <redacted>
---

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.

quoted

+                       ;
+               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