Re: [PATCH v4 08/12] ref-filter: introduce align_atom_parser()
From: Eric Sunshine <hidden>
Date: 2016-06-15 23:08:06
On Sun, Jan 31, 2016 at 12:42 PM, Karthik Nayak [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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 <redacted> ---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'.)
+
+ if (!strtoul_ui(arg, 10, &width))
+ ;
+ else if ((position = parse_align_position(arg)) >= 0)
+ align->position = position;
+ else
+ die(_("unrecognized %%(align) argument: %s"), arg);
+ s++;
+ }
+
+ if (width == ~0U)
+ die(_("positive width expected with the %%(align) atom"));
+ align->width = width;
+ strbuf_list_free(to_free);
+}