Re: [PATCH v6 02/11] trailer: process trailers from stdin and arguments
On Fri, Mar 7, 2014 at 9:08 AM, Dan Carpenter wrote: > On Fri, Mar 07, 2014 at 07:19:15AM +0100, Christian Couder wrote: >> >> Yeah, I don't know why, but these days I find it very hard to review >> style issues in my own code without being distracted. >> And by the way is there a good script to check them? > > Many of these would have been caught with kernel.org's checkpatch.pl > script. > > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl Yeah, I already thought about using it in the past, but I was not sure about its status regarding the git source code because it's use is not suggested in CodingGuidelines, and the discussions about adding it there went nowhere: http://thread.gmane.org/gmane.comp.version-control.git/223698/focus=224653 Thanks, Christian. -- 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 v6 02/11] trailer: process trailers from stdin and arguments
On Fri, Mar 07, 2014 at 07:19:15AM +0100, Christian Couder wrote: > On Wed, Mar 5, 2014 at 11:52 PM, Junio C Hamano wrote: > > > > This round is marked as the sixth, but I still see quite a many > > style issues, which I expect not to see from long timers without > > being told. Somewhat disappointing... > > Yeah, I don't know why, but these days I find it very hard to review > style issues in my own code without being distracted. > And by the way is there a good script to check them? Many of these would have been caught with kernel.org's checkpatch.pl script. http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/plain/scripts/checkpatch.pl regards, dan carpenter -- 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 v6 02/11] trailer: process trailers from stdin and arguments
On Wed, Mar 5, 2014 at 11:52 PM, Junio C Hamano wrote: > > This round is marked as the sixth, but I still see quite a many > style issues, which I expect not to see from long timers without > being told. Somewhat disappointing... Yeah, I don't know why, but these days I find it very hard to review style issues in my own code without being distracted. And by the way is there a good script to check them? Thanks, Christian. -- 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 v6 02/11] trailer: process trailers from stdin and arguments
Christian Couder writes:
> Implement the logic to process trailers from stdin and arguments.
>
> At the beginning trailers from stdin are in their own in_tok
> doubly linked list, and trailers from arguments are in their own
> arg_tok doubly linked list.
>
> The lists are traversed and when an arg_tok should be "applied",
> it is removed from its list and inserted into the in_tok list.
>
> Signed-off-by: Christian Couder
> ---
Thanks.
This round is marked as the sixth, but I still see quite a many
style issues, which I expect not to see from long timers without
being told. Somewhat disappointing...
> trailer.c | 197
> ++
> 1 file changed, 197 insertions(+)
>
> diff --git a/trailer.c b/trailer.c
> index db93a63..a0124f2 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -47,3 +47,200 @@ static size_t alnum_len(const char *buf, size_t len)
> ...
> + if ((where == WHERE_AFTER ? in_tok->next :
> in_tok->previous) == arg_tok)
Overlong line that does not have to be.
if ((where == WHERE_AFTER
? in_tok->next : in_tok->previous) == arg_tok)
or something?
> +static void update_last(struct trailer_item **last)
> +{
> + if (*last)
> + while((*last)->next != NULL)
Style. SP between control keyword and the expression.
> + *last = (*last)->next;
> +}
> +
> +static void update_first(struct trailer_item **first)
> +{
> + if (*first)
> + while((*first)->previous != NULL)
Ditto.
> +static void process_trailers_lists(struct trailer_item **in_tok_first,
> ...
> + /* Process input from end to start */
> + for (in_tok = *in_tok_last; in_tok; in_tok = in_tok->previous) {
> + process_input_token(in_tok, arg_tok_first, WHERE_AFTER);
> + }
Needless brace pair.
> + /* Process input from start to end */
> + for (in_tok = *in_tok_first; in_tok; in_tok = in_tok->next) {
> + process_input_token(in_tok, arg_tok_first, WHERE_BEFORE);
> + }
Ditto.
--
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 v6 02/11] trailer: process trailers from stdin and arguments
Implement the logic to process trailers from stdin and arguments.
At the beginning trailers from stdin are in their own in_tok
doubly linked list, and trailers from arguments are in their own
arg_tok doubly linked list.
The lists are traversed and when an arg_tok should be "applied",
it is removed from its list and inserted into the in_tok list.
Signed-off-by: Christian Couder
---
trailer.c | 197 ++
1 file changed, 197 insertions(+)
diff --git a/trailer.c b/trailer.c
index db93a63..a0124f2 100644
--- a/trailer.c
+++ b/trailer.c
@@ -47,3 +47,200 @@ static size_t alnum_len(const char *buf, size_t len)
len--;
return len;
}
+
+static void free_trailer_item(struct trailer_item *item)
+{
+ free(item->conf.name);
+ free(item->conf.key);
+ free(item->conf.command);
+ free((char *)item->token);
+ free((char *)item->value);
+ free(item);
+}
+
+static void add_arg_to_input_list(struct trailer_item *in_tok,
+ struct trailer_item *arg_tok)
+{
+ if (arg_tok->conf.where == WHERE_AFTER) {
+ arg_tok->next = in_tok->next;
+ in_tok->next = arg_tok;
+ arg_tok->previous = in_tok;
+ if (arg_tok->next)
+ arg_tok->next->previous = arg_tok;
+ } else {
+ arg_tok->previous = in_tok->previous;
+ in_tok->previous = arg_tok;
+ arg_tok->next = in_tok;
+ if (arg_tok->previous)
+ arg_tok->previous->next = arg_tok;
+ }
+}
+
+static int check_if_different(struct trailer_item *in_tok,
+ struct trailer_item *arg_tok,
+ int alnum_len, int check_all)
+{
+ enum action_where where = arg_tok->conf.where;
+ do {
+ if (!in_tok)
+ return 1;
+ if (same_trailer(in_tok, arg_tok, alnum_len))
+ return 0;
+ /*
+* if we want to add a trailer after another one,
+* we have to check those before this one
+*/
+ in_tok = (where == WHERE_AFTER) ? in_tok->previous :
in_tok->next;
+ } while (check_all);
+ return 1;
+}
+
+static void apply_arg_if_exists(struct trailer_item *in_tok,
+ struct trailer_item *arg_tok,
+ int alnum_len)
+{
+ switch (arg_tok->conf.if_exists) {
+ case EXISTS_DO_NOTHING:
+ free_trailer_item(arg_tok);
+ break;
+ case EXISTS_OVERWRITE:
+ free((char *)in_tok->value);
+ in_tok->value = xstrdup(arg_tok->value);
+ free_trailer_item(arg_tok);
+ break;
+ case EXISTS_ADD:
+ add_arg_to_input_list(in_tok, arg_tok);
+ break;
+ case EXISTS_ADD_IF_DIFFERENT:
+ if (check_if_different(in_tok, arg_tok, alnum_len, 1))
+ add_arg_to_input_list(in_tok, arg_tok);
+ else
+ free_trailer_item(arg_tok);
+ break;
+ case EXISTS_ADD_IF_DIFFERENT_NEIGHBOR:
+ if (check_if_different(in_tok, arg_tok, alnum_len, 0))
+ add_arg_to_input_list(in_tok, arg_tok);
+ else
+ free_trailer_item(arg_tok);
+ break;
+ }
+}
+
+static void remove_from_list(struct trailer_item *item,
+struct trailer_item **first)
+{
+ if (item->next)
+ item->next->previous = item->previous;
+ if (item->previous)
+ item->previous->next = item->next;
+ else
+ *first = item->next;
+}
+
+static struct trailer_item *remove_first(struct trailer_item **first)
+{
+ struct trailer_item *item = *first;
+ *first = item->next;
+ if (item->next) {
+ item->next->previous = NULL;
+ item->next = NULL;
+ }
+ return item;
+}
+
+static void process_input_token(struct trailer_item *in_tok,
+ struct trailer_item **arg_tok_first,
+ enum action_where where)
+{
+ struct trailer_item *arg_tok;
+ struct trailer_item *next_arg;
+
+ int tok_alnum_len = alnum_len(in_tok->token, strlen(in_tok->token));
+ for (arg_tok = *arg_tok_first; arg_tok; arg_tok = next_arg) {
+ next_arg = arg_tok->next;
+ if (same_token(in_tok, arg_tok, tok_alnum_len) &&
+ arg_tok->conf.where == where) {
+ remove_from_list(arg_tok, arg_tok_first);
+ apply_arg_if_exists(in_tok, arg_tok, tok_alnum_len);
+ /*
+* If arg has been added to input,
+* then we n
