Re: [PATCH v2 2/6] trailer: use list.h for doubly-linked list

[Junio C Hamano]

Jonathan Tan  writes:

> Replace the existing handwritten implementation of a doubly-linked list
> in trailer.c with the functions and macros from list.h. This
> significantly simplifies the code.
> ---

The handcrafted one in trailer.c somehow did not use the common
practice of using a doubly-linked cycle as a doubly-linked list with
a designated fake element as the pointers to the first and to the
last elements of the list (instead it used NULL as the "this is the
end in this direction" convention just like a common singly-linked
list), and this update removes the need for special cases handling
the elements at the beginning and at the end that comes from that
choice by switching to list.h macros.  update_last/update_first can
go, two parameters that were passed to point at the variables for
the beginning and the end can go, the special cases for the initial
condition in add_trailer_item() can go, all thanks to this change.

Very nice.

>  trailer.c | 258 
> ++
>  1 file changed, 91 insertions(+), 167 deletions(-)

Re: [PATCH v2 2/6] trailer: use list.h for doubly-linked list

[Jakub Narębski]

W dniu 13.10.2016 o 01:40, Jonathan Tan pisze:
> Replace the existing handwritten implementation of a doubly-linked list
> in trailer.c with the functions and macros from list.h. This
> significantly simplifies the code.
> ---
>  trailer.c | 258 
> ++
>  1 file changed, 91 insertions(+), 167 deletions(-)

Very nice gains!

BTW. could you sign (Signed-off-by) your patches? TIA.

Best,
-- 
Jakub Narębski

[PATCH v2 2/6] trailer: use list.h for doubly-linked list

[Jonathan Tan]

Replace the existing handwritten implementation of a doubly-linked list
in trailer.c with the functions and macros from list.h. This
significantly simplifies the code.
---
 trailer.c | 258 ++
 1 file changed, 91 insertions(+), 167 deletions(-)

diff --git a/trailer.c b/trailer.c
index 1f191b2..0afa240 100644
--- a/trailer.c
+++ b/trailer.c
@@ -4,6 +4,7 @@
 #include "commit.h"
 #include "tempfile.h"
 #include "trailer.h"
+#include "list.h"
 /*
  * Copyright (c) 2013, 2014 Christian Couder 
  */
@@ -25,19 +26,24 @@ struct conf_info {
 static struct conf_info default_conf_info;
 
 struct trailer_item {
-   struct trailer_item *previous;
-   struct trailer_item *next;
+   struct list_head list;
char *token;
char *value;
struct conf_info conf;
 };
 
-static struct trailer_item *first_conf_item;
+LIST_HEAD(conf_head);
 
 static char *separators = ":";
 
 #define TRAILER_ARG_STRING "$ARG"
 
+/* Iterate over the elements of the list. */
+#define list_for_each_dir(pos, head, is_reverse) \
+   for (pos = is_reverse ? (head)->prev : (head)->next; \
+   pos != (head); \
+   pos = is_reverse ? pos->prev : pos->next)
+
 static int after_or_end(enum action_where where)
 {
return (where == WHERE_AFTER) || (where == WHERE_END);
@@ -120,101 +126,49 @@ static void print_tok_val(FILE *outfile, const char 
*tok, const char *val)
fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
 }
 
-static void print_all(FILE *outfile, struct trailer_item *first, int 
trim_empty)
+static void print_all(FILE *outfile, struct list_head *head, int trim_empty)
 {
+   struct list_head *pos;
struct trailer_item *item;
-   for (item = first; item; item = item->next) {
+   list_for_each(pos, head) {
+   item = list_entry(pos, struct trailer_item, list);
if (!trim_empty || strlen(item->value) > 0)
print_tok_val(outfile, item->token, item->value);
}
 }
 
-static void update_last(struct trailer_item **last)
-{
-   if (*last)
-   while ((*last)->next != NULL)
-   *last = (*last)->next;
-}
-
-static void update_first(struct trailer_item **first)
-{
-   if (*first)
-   while ((*first)->previous != NULL)
-   *first = (*first)->previous;
-}
-
 static void add_arg_to_input_list(struct trailer_item *on_tok,
- struct trailer_item *arg_tok,
- struct trailer_item **first,
- struct trailer_item **last)
-{
-   if (after_or_end(arg_tok->conf.where)) {
-   arg_tok->next = on_tok->next;
-   on_tok->next = arg_tok;
-   arg_tok->previous = on_tok;
-   if (arg_tok->next)
-   arg_tok->next->previous = arg_tok;
-   update_last(last);
-   } else {
-   arg_tok->previous = on_tok->previous;
-   on_tok->previous = arg_tok;
-   arg_tok->next = on_tok;
-   if (arg_tok->previous)
-   arg_tok->previous->next = arg_tok;
-   update_first(first);
-   }
+ struct trailer_item *arg_tok)
+{
+   if (after_or_end(arg_tok->conf.where))
+   list_add(&arg_tok->list, &on_tok->list);
+   else
+   list_add_tail(&arg_tok->list, &on_tok->list);
 }
 
 static int check_if_different(struct trailer_item *in_tok,
  struct trailer_item *arg_tok,
- int check_all)
+ int check_all,
+ struct list_head *head)
 {
enum action_where where = arg_tok->conf.where;
+   struct list_head *next_head;
do {
-   if (!in_tok)
-   return 1;
if (same_trailer(in_tok, arg_tok))
return 0;
/*
 * if we want to add a trailer after another one,
 * we have to check those before this one
 */
-   in_tok = after_or_end(where) ? in_tok->previous : in_tok->next;
+   next_head = after_or_end(where) ? in_tok->list.prev
+   : in_tok->list.next;
+   if (next_head == head)
+   break;
+   in_tok = list_entry(next_head, struct trailer_item, list);
} while (check_all);
return 1;
 }
 
-static void remove_from_list(struct trailer_item *item,
-struct trailer_item **first,
-struct trailer_item **last)
-{
-   struct trailer_item *next = item->next;
-   struct trailer_item *previous = item->previous;
-
-   if (next) {
-   item->next->previous = previous;
-