Re: [PATCH] attr: fix off-by-one directory component length calculation
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 16, 2013, 19:07 UTC
- Message-ID
- <7vzk09vtdr.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20130116060238.GA29523@duynguyen-vnpc.dek-tpc.internal>
Duy Nguyen <pclouds@gmail.com> writes:
Show 12 quoted lines
> OK I get your point now. Something like this? > > -- 8< -- > Subject: [PATCH] attr: avoid calling find_basename() twice per path > > find_basename() is only used inside collect_all_attrs(), called once > in prepare_attr_stack, then again after prepare_attr_stack() > returns. Both calls return exact same value. Reorder the code to do > the same task once. Also avoid strlen() because we knows the length > after finding basename. > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Yeah, I think this is a nice code reduction, readability improvement and micro optimization rolled into one.
Show 74 quoted lines
> attr.c | 45 ++++++++++++++++++---------------------------
> 1 file changed, 18 insertions(+), 27 deletions(-)
>
> diff --git a/attr.c b/attr.c
> index cfc6748..880f862 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -564,32 +564,12 @@ static void bootstrap_attr_stack(void)
> attr_stack = elem;
> }
>
> -static const char *find_basename(const char *path)
> -{
> - const char *cp, *last_slash = NULL;
> -
> - for (cp = path; *cp; cp++) {
> - if (*cp == '/' && cp[1])
> - last_slash = cp;
> - }
> - return last_slash ? last_slash + 1 : path;
> -}
> -
> -static void prepare_attr_stack(const char *path)
> +static void prepare_attr_stack(const char *path, int dirlen)
> {
> struct attr_stack *elem, *info;
> - int dirlen, len;
> + int len;
> const char *cp;
>
> - dirlen = find_basename(path) - path;
> -
> - /*
> - * find_basename() includes the trailing slash, but we do
> - * _not_ want it.
> - */
> - if (dirlen)
> - dirlen--;
> -
> /*
> * At the bottom of the attribute stack is the built-in
> * set of attribute definitions, followed by the contents
> @@ -769,15 +749,26 @@ static int macroexpand_one(int attr_nr, int rem)
> static void collect_all_attrs(const char *path)
> {
> struct attr_stack *stk;
> - int i, pathlen, rem;
> - const char *basename;
> + int i, pathlen, rem, dirlen;
> + const char *basename, *cp, *last_slash = NULL;
> +
> + for (cp = path; *cp; cp++) {
> + if (*cp == '/' && cp[1])
> + last_slash = cp;
> + }
> + pathlen = cp - path;
> + if (last_slash) {
> + basename = last_slash + 1;
> + dirlen = last_slash - path;
> + } else {
> + basename = path;
> + dirlen = 0;
> + }
>
> - prepare_attr_stack(path);
> + prepare_attr_stack(path, dirlen);
> for (i = 0; i < attr_nr; i++)
> check_all_attr[i].value = ATTR__UNKNOWN;
>
> - basename = find_basename(path);
> - pathlen = strlen(path);
> rem = attr_nr;
> for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)
> rem = fill(path, pathlen, basename, stk, rem);