Re: [PATCH v5 01/14] Add data structures and basic functions for commit trailers
On Sun, Feb 9, 2014 at 8:48 AM, Christian Couder
wrote:
> From: Junio C Hamano
>>
>> Christian Couder writes:
>>
>>> +static inline int same_trailer(struct trailer_item *a, struct trailer_item
>>> *b, int alnum_len)
>>> +{
>>> +return same_token(a, b, alnum_len) && same_value(a, b);
>>> +}
>>
>> All these "inlines" look premature optimization that can be
>> delegated to any decent compiler, don't they?
>
> Yeah, but as Eric suggested to add them like in header files and you
> did not reply to him, I thought you agreed with him.
> I will remove them.
If these functions are used only by trailer.c, then it would make
sense to move them from trailer.h to trailer.c so that they don't get
defined unnecessarily by each .c file which includes trailer.h.
>>> +/* Get the length of buf from its beginning until its last alphanumeric
>>> character */
>>> +static inline size_t alnum_len(const char *buf, int len)
>>> +{
>>> +while (--len >= 0 && !isalnum(buf[len]));
>>> +return (size_t) len + 1;
>>
>> This is somewhat unfortunate. if the caller wants to receive
>> size_t, perhaps it should be passing in size_t (or ssize_t) to the
>> function? Hard to guess without an actual caller, though.
>
> Ok, I will make it return an int.
Why not adjust the loop condition slightly instead so that you can
continue to accept and return size_t?
--
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 v5 01/14] Add data structures and basic functions for commit trailers
From: Junio C Hamano
>
> Christian Couder writes:
>
>> +enum action_if_exist if_exist;
>> +enum action_if_missing if_missing;
>
> Probably "if_exists" is more gramatically correct.
>
> if (x->if_exists) {
> ... do this ...
> }
>
> would read well, but not "x->if_exist".
Ok, I will use "if_exists" instead of "if_exist" and also:
enum action_if_exists { EXISTS_ADD_IF_DIFFERENT,
EXISTS_ADD_IF_DIFFERENT_NEIGHBOR,
EXISTS_ADD, EXISTS_OVERWRITE, EXISTS_DO_NOTHING };
instead of:
enum action_if_exist { EXIST_ADD_IF_DIFFERENT, EXIST_ADD_IF_DIFFERENT_NEIGHBOR,
EXIST_ADD, EXIST_OVERWRITE, EXIST_DO_NOTHING };
to be consistent.
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 v5 01/14] Add data structures and basic functions for commit trailers
From: Junio C Hamano
>
> Christian Couder writes:
>
>> +enum action_where { WHERE_AFTER, WHERE_BEFORE };
>> +enum action_if_exist { EXIST_ADD_IF_DIFFERENT,
>> EXIST_ADD_IF_DIFFERENT_NEIGHBOR,
>> + EXIST_ADD, EXIST_OVERWRITE, EXIST_DO_NOTHING };
>> +enum action_if_missing { MISSING_ADD, MISSING_DO_NOTHING };
>
> All these names and "conf_info" below are not named to be specific
> to this little tool. Can I assume that these will never be exposed
> to the rest of the system? If so, they are fine.
Yeah, I don't plan them to be exposed to other files.
>> +struct conf_info {
>> +char *name;
>> +char *key;
>> +char *command;
>> +enum action_where where;
>> +enum action_if_exist if_exist;
>> +enum action_if_missing if_missing;
>
> It still feels somewhat strange. It is true that an item can be
> either "exist" or "missing" and it is understandable that it tempts
> you to split that into two, but EXIST_OVERWRITE will not trigger
> either WHERE_AFTER or WHERE_BEFORE action.
Yeah, it's true that WHERE_AFTER/WHERE_BEFORE does not make sense for
EXIST_OVERWRITE, EXIST_DO_NOTHING and MISSING_DO_NOTHING, but it's a
fact of life that sometimes some options do not make sense with
others.
>> +static inline int same_token(struct trailer_item *a, struct trailer_item
>> *b, int alnum_len)
>> +{
>> +return !strncasecmp(a->token, b->token, alnum_len);
>> +}
>> +
>> +static inline int same_value(struct trailer_item *a, struct trailer_item *b)
>> +{
>> +return !strcasecmp(a->value, b->value);
>> +}
>> +
>> +static inline int same_trailer(struct trailer_item *a, struct trailer_item
>> *b, int alnum_len)
>> +{
>> +return same_token(a, b, alnum_len) && same_value(a, b);
>> +}
>
> All these "inlines" look premature optimization that can be
> delegated to any decent compiler, don't they?
Yeah, but as Eric suggested to add them like in header files and you
did not reply to him, I thought you agreed with him.
I will remove them.
>> +/* Get the length of buf from its beginning until its last alphanumeric
>> character */
>> +static inline size_t alnum_len(const char *buf, int len)
>> +{
>> +while (--len >= 0 && !isalnum(buf[len]));
>
> Style:
>
> while (--len >= 0 && !isalnum(buf[len]))
> ;
>
> You may add a comment on the empty statement to make it stand out
> even more, i.e.
>
> ; /* nothing */
Ok, I will do that.
>> +return (size_t) len + 1;
>
> This is somewhat unfortunate. if the caller wants to receive
> size_t, perhaps it should be passing in size_t (or ssize_t) to the
> function? Hard to guess without an actual caller, though.
Ok, I will make it return an int.
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 v5 01/14] Add data structures and basic functions for commit trailers
Christian Couder writes:
> + enum action_if_exist if_exist;
> + enum action_if_missing if_missing;
Probably "if_exists" is more gramatically correct.
if (x->if_exists) {
... do this ...
}
would read well, but not "x->if_exist".
--
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 v5 01/14] Add data structures and basic functions for commit trailers
Christian Couder writes:
> We will use a doubly linked list to store all information
> about trailers and their configuration.
>
> This way we can easily remove or add trailers to or from
> trailer lists while traversing the lists in either direction.
>
> Signed-off-by: Christian Couder
> ---
> Makefile | 1 +
> trailer.c | 48
> 2 files changed, 49 insertions(+)
> create mode 100644 trailer.c
>
> diff --git a/Makefile b/Makefile
> index b4af1e2..ec90feb 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -871,6 +871,7 @@ LIB_OBJS += submodule.o
> LIB_OBJS += symlinks.o
> LIB_OBJS += tag.o
> LIB_OBJS += trace.o
> +LIB_OBJS += trailer.o
> LIB_OBJS += transport.o
> LIB_OBJS += transport-helper.o
> LIB_OBJS += tree-diff.o
> diff --git a/trailer.c b/trailer.c
> new file mode 100644
> index 000..f129b5a
> --- /dev/null
> +++ b/trailer.c
> @@ -0,0 +1,48 @@
> +#include "cache.h"
> +/*
> + * Copyright (c) 2013 Christian Couder
> + */
> +
> +enum action_where { WHERE_AFTER, WHERE_BEFORE };
> +enum action_if_exist { EXIST_ADD_IF_DIFFERENT,
> EXIST_ADD_IF_DIFFERENT_NEIGHBOR,
> +EXIST_ADD, EXIST_OVERWRITE, EXIST_DO_NOTHING };
> +enum action_if_missing { MISSING_ADD, MISSING_DO_NOTHING };
All these names and "conf_info" below are not named to be specific
to this little tool. Can I assume that these will never be exposed
to the rest of the system? If so, they are fine.
> +struct conf_info {
> + char *name;
> + char *key;
> + char *command;
> + enum action_where where;
> + enum action_if_exist if_exist;
> + enum action_if_missing if_missing;
It still feels somewhat strange. It is true that an item can be
either "exist" or "missing" and it is understandable that it tempts
you to split that into two, but EXIST_OVERWRITE will not trigger
either WHERE_AFTER or WHERE_BEFORE action.
> +};
> +
> +struct trailer_item {
> + struct trailer_item *previous;
> + struct trailer_item *next;
> + const char *token;
> + const char *value;
> + struct conf_info *conf;
> +};
> +
> +static inline int same_token(struct trailer_item *a, struct trailer_item *b,
> int alnum_len)
> +{
> + return !strncasecmp(a->token, b->token, alnum_len);
> +}
> +
> +static inline int same_value(struct trailer_item *a, struct trailer_item *b)
> +{
> + return !strcasecmp(a->value, b->value);
> +}
> +
> +static inline int same_trailer(struct trailer_item *a, struct trailer_item
> *b, int alnum_len)
> +{
> + return same_token(a, b, alnum_len) && same_value(a, b);
> +}
All these "inlines" look premature optimization that can be
delegated to any decent compiler, don't they?
> +/* Get the length of buf from its beginning until its last alphanumeric
> character */
> +static inline size_t alnum_len(const char *buf, int len)
> +{
> + while (--len >= 0 && !isalnum(buf[len]));
Style:
while (--len >= 0 && !isalnum(buf[len]))
;
You may add a comment on the empty statement to make it stand out
even more, i.e.
; /* nothing */
> + return (size_t) len + 1;
This is somewhat unfortunate. if the caller wants to receive
size_t, perhaps it should be passing in size_t (or ssize_t) to the
function? Hard to guess without an actual caller, though.
> +}
--
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
