From: Junio C Hamano <[email protected]>
>
> Christian Couder <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
