threads / patch / 28084

patch, 7 partsComments and refactoring in attr.c

Subject: [PATCH 0/7] Comments and refactoring in attr.c

## tl;dr

8 messages between Aug 12, 2011 and Aug 12, 2011. Diffs are folded; open one to read it.

replies: 7people: 1as markdown or json

Michael Haggerty· Aug 12, 2011, 21:43 UTC · lore

This patch series adds comments for some non-obvious things in attr.c, simplifies the interface of parse_attr(), and unrolls the loop over passes in parse_attr_line() (which IMO makes the code easier to understand). It does not change the observable behavior in any way.

The patch series is against master, but can be rebased to next without conflicts.

Michael Haggerty (7):
  Add a file comment
  Document struct match_attr
  Increment num_attr in parse_attr_line(), not parse_attr()
  Change parse_attr() to take a pointer to struct attr_state
  Determine the start of the states outside of the pass loop
  Change while loop into for loop
  Unroll the loop over passes

 attr.c |  113 +++++++++++++++++++++++++++++++++++----------------------------
 1 files changed, 63 insertions(+), 50 deletions(-)

-- 
1.7.6.8.gd2879

Michael Haggerty· Aug 12, 2011, 21:43 UTC · re: Michael Haggerty · lore

[PATCH 1/7] Add a file comment

Consolidate here a few general comments plus links to other documentation. Delete a comment with an out-of-date description of the .gitattributes file format.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 attr.c |   26 ++++++++++----------------
 1 files changed, 10 insertions(+), 16 deletions(-)

Show changes to attr.c +10 −16

diff --git a/attr.c b/attr.c
index f6b3f7e..6bc7ae9 100644
--- a/attr.c
+++ b/attr.c
@@ -1,3 +1,12 @@
+/*
+ * Handle git attributes.  See gitattributes(5) for a description of
+ * the file syntax, and Documentation/technical/api-gitattributes.txt
+ * for a description of the API.
+ *
+ * One basic design decision here is that we are not going to support
+ * an insanely large number of attributes.
+ */
+
 #define NO_THE_INDEX_COMPATIBILITY_MACROS
 #include "cache.h"
 #include "exec_cmd.h"
@@ -13,12 +22,7 @@ static const char git_attr__unknown[] = "(builtin)unknown";
 
 static const char *attributes_file;
 
-/*
- * The basic design decision here is that we are not going to have
- * insanely large number of attributes.
- *
- * This is a randomly chosen prime.
- */
+/* This is a randomly chosen prime. */
 #define HASHSIZE 257
 
 #ifndef DEBUG_ATTR
@@ -103,16 +107,6 @@ struct git_attr *git_attr(const char *name)
 	return git_attr_internal(name, strlen(name));
 }
 
-/*
- * .gitattributes file is one line per record, each of which is
- *
- * (1) glob pattern.
- * (2) whitespace
- * (3) whitespace separated list of attribute names, each of which
- *     could be prefixed with '-' to mean "set to false", '!' to mean
- *     "unset".
- */
-
 /* What does a matched pattern decide? */
 struct attr_state {
 	struct git_attr *attr;

-- 
1.7.6.8.gd2879

Michael Haggerty· Aug 12, 2011, 21:43 UTC · re: Michael Haggerty · lore

[PATCH 2/7] Document struct match_attr

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 attr.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

Show changes to attr.c +14 −0

diff --git a/attr.c b/attr.c
index 6bc7ae9..c33e413 100644
--- a/attr.c
+++ b/attr.c
@@ -113,6 +113,20 @@ struct attr_state {
 	const char *setto;
 };
 
+/*
+ * One rule, as from a .gitattributes file.
+ *
+ * If is_macro is true, then u.attr is a pointer to the git_attr being
+ * defined.
+ *
+ * If is_macro is false, then u.pattern points at the filename pattern
+ * to which the rule applies.  (The memory pointed to is part of the
+ * memory block allocated for the match_attr instance.)
+ *
+ * In either case, num_attr is the number of attributes affected by
+ * this rule, and state is an array listing them.  The attributes are
+ * listed as they appear in the file (macros unexpanded).
+ */
 struct match_attr {
 	union {
 		char *pattern;

-- 
1.7.6.8.gd2879

Michael Haggerty· Aug 12, 2011, 21:43 UTC · re: Michael Haggerty · lore

[PATCH 3/7] Increment num_attr in parse_attr_line(), not parse_attr()

num_attr is incremented iff parse_attr() returns non-NULL. So do the counting in parse_attr_line() instead of within parse_attr(). This allows an integer rather than a pointer to an integer to be passed to parse_attr().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 attr.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

Show changes to attr.c +4 −4

diff --git a/attr.c b/attr.c
index c33e413..cac550d 100644
--- a/attr.c
+++ b/attr.c
@@ -140,7 +140,7 @@ struct match_attr {
 static const char blank[] = " \t\r\n";
 
 static const char *parse_attr(const char *src, int lineno, const char *cp,
-			      int *num_attr, struct match_attr *res)
+			      int num_attr, struct match_attr *res)
 {
 	const char *ep, *equals;
 	int len;
@@ -167,7 +167,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
 	} else {
 		struct attr_state *e;
 
-		e = &(res->state[*num_attr]);
+		e = &(res->state[num_attr]);
 		if (*cp == '-' || *cp == '!') {
 			e->setto = (*cp == '-') ? ATTR__FALSE : ATTR__UNSET;
 			cp++;
@@ -180,7 +180,6 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
 		}
 		e->attr = git_attr_internal(cp, len);
 	}
-	(*num_attr)++;
 	return ep + strspn(ep, blank);
 }
 
@@ -226,9 +225,10 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 		cp = name + namelen;
 		cp = cp + strspn(cp, blank);
 		while (*cp) {
-			cp = parse_attr(src, lineno, cp, &num_attr, res);
+			cp = parse_attr(src, lineno, cp, num_attr, res);
 			if (!cp)
 				return NULL;
+			num_attr++;
 		}
 		if (pass)
 			break;

-- 
1.7.6.8.gd2879

Michael Haggerty· Aug 12, 2011, 21:43 UTC · re: Michael Haggerty · lore

[PATCH 4/7] Change parse_attr() to take a pointer to struct attr_state

parse_attr() only needs access to the attr_state to which it should store its results, not to the whole match_attr structure. This change also removes the need for it to know num_attr. Change its signature accordingly and add a comment.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 attr.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

Show changes to attr.c +11 −6

diff --git a/attr.c b/attr.c
index cac550d..f23f62a 100644
--- a/attr.c
+++ b/attr.c
@@ -139,8 +139,15 @@ struct match_attr {
 
 static const char blank[] = " \t\r\n";
 
+/*
+ * Parse a whitespace-delimited attribute state (i.e., "attr",
+ * "-attr", "!attr", or "attr=value") from the string starting at src.
+ * If e is not NULL, write the results to *e.  Return a pointer to the
+ * remainder of the string (with leading whitespace removed), or NULL
+ * if there was an error.
+ */
 static const char *parse_attr(const char *src, int lineno, const char *cp,
-			      int num_attr, struct match_attr *res)
+			      struct attr_state *e)
 {
 	const char *ep, *equals;
 	int len;
@@ -153,7 +160,7 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
 		len = equals - cp;
 	else
 		len = ep - cp;
-	if (!res) {
+	if (!e) {
 		if (*cp == '-' || *cp == '!') {
 			cp++;
 			len--;
@@ -165,9 +172,6 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
 			return NULL;
 		}
 	} else {
-		struct attr_state *e;
-
-		e = &(res->state[num_attr]);
 		if (*cp == '-' || *cp == '!') {
 			e->setto = (*cp == '-') ? ATTR__FALSE : ATTR__UNSET;
 			cp++;
@@ -225,7 +229,8 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 		cp = name + namelen;
 		cp = cp + strspn(cp, blank);
 		while (*cp) {
-			cp = parse_attr(src, lineno, cp, num_attr, res);
+			cp = parse_attr(src, lineno, cp,
+				pass ? &(res->state[num_attr]) : NULL);
 			if (!cp)
 				return NULL;
 			num_attr++;

-- 
1.7.6.8.gd2879

Michael Haggerty· Aug 12, 2011, 21:43 UTC · re: Michael Haggerty · lore

[PATCH 5/7] Determine the start of the states outside of the pass loop

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 attr.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

Show changes to attr.c +5 −3

diff --git a/attr.c b/attr.c
index f23f62a..a7d1aa9 100644
--- a/attr.c
+++ b/attr.c
@@ -192,7 +192,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 {
 	int namelen;
 	int num_attr;
-	const char *cp, *name;
+	const char *cp, *name, *states;
 	struct match_attr *res = NULL;
 	int pass;
 	int is_macro;
@@ -223,11 +223,13 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 	else
 		is_macro = 0;
 
+	states = name + namelen;
+	states += strspn(states, blank);
+
 	for (pass = 0; pass < 2; pass++) {
 		/* pass 0 counts and allocates, pass 1 fills */
 		num_attr = 0;
-		cp = name + namelen;
-		cp = cp + strspn(cp, blank);
+		cp = states;
 		while (*cp) {
 			cp = parse_attr(src, lineno, cp,
 				pass ? &(res->state[num_attr]) : NULL);

-- 
1.7.6.8.gd2879

Michael Haggerty· Aug 12, 2011, 21:43 UTC · re: Michael Haggerty · lore

[PATCH 6/7] Change while loop into for loop

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 attr.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

Show changes to attr.c +1 −4

diff --git a/attr.c b/attr.c
index a7d1aa9..b56542e 100644
--- a/attr.c
+++ b/attr.c
@@ -228,14 +228,11 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 
 	for (pass = 0; pass < 2; pass++) {
 		/* pass 0 counts and allocates, pass 1 fills */
-		num_attr = 0;
-		cp = states;
-		while (*cp) {
+		for (cp = states, num_attr = 0; *cp; num_attr++) {
 			cp = parse_attr(src, lineno, cp,
 				pass ? &(res->state[num_attr]) : NULL);
 			if (!cp)
 				return NULL;
-			num_attr++;
 		}
 		if (pass)
 			break;

-- 
1.7.6.8.gd2879

Michael Haggerty· Aug 12, 2011, 21:43 UTC · re: Michael Haggerty · lore

[PATCH 7/7] Unroll the loop over passes

The passes no longer share much code, and the unrolled code is easier to understand.

Use a new index variable instead of num_attr for the second loop, as we are no longer counting attributes but rather indexing through them.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 attr.c |   51 ++++++++++++++++++++++++++-------------------------
 1 files changed, 26 insertions(+), 25 deletions(-)

Show changes to attr.c +26 −25

diff --git a/attr.c b/attr.c
index b56542e..6abaaec 100644
--- a/attr.c
+++ b/attr.c
@@ -191,10 +191,9 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 					  int lineno, int macro_ok)
 {
 	int namelen;
-	int num_attr;
+	int num_attr, i;
 	const char *cp, *name, *states;
 	struct match_attr *res = NULL;
-	int pass;
 	int is_macro;
 
 	cp = line + strspn(line, blank);
@@ -226,30 +225,32 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
 	states = name + namelen;
 	states += strspn(states, blank);
 
-	for (pass = 0; pass < 2; pass++) {
-		/* pass 0 counts and allocates, pass 1 fills */
-		for (cp = states, num_attr = 0; *cp; num_attr++) {
-			cp = parse_attr(src, lineno, cp,
-				pass ? &(res->state[num_attr]) : NULL);
-			if (!cp)
-				return NULL;
-		}
-		if (pass)
-			break;
-		res = xcalloc(1,
-			      sizeof(*res) +
-			      sizeof(struct attr_state) * num_attr +
-			      (is_macro ? 0 : namelen + 1));
-		if (is_macro)
-			res->u.attr = git_attr_internal(name, namelen);
-		else {
-			res->u.pattern = (char *)&(res->state[num_attr]);
-			memcpy(res->u.pattern, name, namelen);
-			res->u.pattern[namelen] = 0;
-		}
-		res->is_macro = is_macro;
-		res->num_attr = num_attr;
+	/* First pass to count the attr_states */
+	for (cp = states, num_attr = 0; *cp; num_attr++) {
+		cp = parse_attr(src, lineno, cp, NULL);
+		if (!cp)
+			return NULL;
 	}
+
+	res = xcalloc(1,
+		      sizeof(*res) +
+		      sizeof(struct attr_state) * num_attr +
+		      (is_macro ? 0 : namelen + 1));
+	if (is_macro)
+		res->u.attr = git_attr_internal(name, namelen);
+	else {
+		res->u.pattern = (char *)&(res->state[num_attr]);
+		memcpy(res->u.pattern, name, namelen);
+		res->u.pattern[namelen] = 0;
+	}
+	res->is_macro = is_macro;
+	res->num_attr = num_attr;
+
+	/* Second pass to fill the attr_states */
+	for (cp = states, i = 0; *cp; i++) {
+		cp = parse_attr(src, lineno, cp, &(res->state[i]));
+	}
+
 	return res;
 }

-- 
1.7.6.8.gd2879