Re: [PATCH v4 08/12] ref-filter: introduce align_atom_parser()

On Fri, Feb 5, 2016 at 5:18 AM, Eric Sunshine [off-list ref] wrote:

On Sun, Jan 31, 2016 at 12:42 PM, 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 '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'.)

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