Re: [PATCH v3 10/15] ref-filter: introduce parse_align_position()

[Karthik Nayak]

On Tue, Jan 26, 2016 at 3:19 AM, Eric Sunshine  wrote:
> On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak  wrote:
>> From align_atom_parser() form parse_align_position() which given a
>> string would give us the alignment position. This is a preparatory
>> patch as to introduce prefixes for the %(align) atom and avoid
>> redundancy in the code.
>>
>> Helped-by: Eric Sunshine 
>> Signed-off-by: Karthik Nayak 
>> ---
>> diff --git a/ref-filter.c b/ref-filter.c
>> @@ -74,6 +74,17 @@ 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;
>> +}
>
> This code was just moved in patch 9/15 and is being relocated again
> here in patch 10/15. If you change the order of the patches so that
> this preparatory refactoring is done first, the diff of the "introduce
> align_atom_parser()" patch will become smaller and be a bit easier to
> review. (Plus it just makes sense to do preparation first.)
>

It does reduce the diff as you said, I have swapped the order as you suggested.

-- 
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 10/15] ref-filter: introduce parse_align_position()

[Eric Sunshine]

On Tue, Jan 5, 2016 at 3:03 AM, Karthik Nayak  wrote:
> From align_atom_parser() form parse_align_position() which given a
> string would give us the alignment position. This is a preparatory
> patch as to introduce prefixes for the %(align) atom and avoid
> redundancy in the code.
>
> Helped-by: Eric Sunshine 
> Signed-off-by: Karthik Nayak 
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -74,6 +74,17 @@ 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;
> +}

This code was just moved in patch 9/15 and is being relocated again
here in patch 10/15. If you change the order of the patches so that
this preparatory refactoring is done first, the diff of the "introduce
align_atom_parser()" patch will become smaller and be a bit easier to
review. (Plus it just makes sense to do preparation first.)

>  static void align_atom_parser(struct used_atom *atom)
>  {
> struct align *align = >u.align;
> @@ -90,16 +101,13 @@ static void align_atom_parser(struct used_atom *atom)
> align->position = ALIGN_LEFT;
>
> while (*s) {
> +   int position;
> buf = s[0]->buf;
>
> if (!strtoul_ui(buf, 10, (unsigned int *)))
> ;
> -   else if (!strcmp(buf, "left"))
> -   align->position = ALIGN_LEFT;
> -   else if (!strcmp(buf, "right"))
> -   align->position = ALIGN_RIGHT;
> -   else if (!strcmp(buf, "middle"))
> -   align->position = ALIGN_MIDDLE;
> +   else if ((position = parse_align_position(buf)) >= 0)
> +   align->position = position;
> else
> die(_("unrecognized %%(align) argument: %s"), buf);
> s++;
> --
> 2.6.4
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3 10/15] ref-filter: introduce parse_align_position()

[Karthik Nayak]

>From align_atom_parser() form parse_align_position() which given a
string would give us the alignment position. This is a preparatory
patch as to introduce prefixes for the %(align) atom and avoid
redundancy in the code.

Helped-by: Eric Sunshine 
Signed-off-by: Karthik Nayak 
---
 ref-filter.c | 20 ++--
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index fa081a8..ccad4c3 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -74,6 +74,17 @@ static void color_atom_parser(struct used_atom *atom)
die(_("invalid color value: %s"), atom->u.color);
 }
 
+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 = >u.align;
@@ -90,16 +101,13 @@ static void align_atom_parser(struct used_atom *atom)
align->position = ALIGN_LEFT;
 
while (*s) {
+   int position;
buf = s[0]->buf;
 
if (!strtoul_ui(buf, 10, (unsigned int *)))
;
-   else if (!strcmp(buf, "left"))
-   align->position = ALIGN_LEFT;
-   else if (!strcmp(buf, "right"))
-   align->position = ALIGN_RIGHT;
-   else if (!strcmp(buf, "middle"))
-   align->position = ALIGN_MIDDLE;
+   else if ((position = parse_align_position(buf)) >= 0)
+   align->position = position;
else
die(_("unrecognized %%(align) argument: %s"), buf);
s++;
-- 
2.6.4

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html