Junio C Hamano [off-list ref] writes:
Adam Spiers [off-list ref] writes:
quoted
diff --git a/builtin/clean.c b/builtin/clean.c
index 0c7b3d0..bd18b88 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -97,9 +97,10 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
if (!ignored)
setup_standard_excludes(&dir);
+ add_exclude_list(&dir, EXC_CMDL);
for (i = 0; i < exclude_list.nr; i++)
add_exclude(exclude_list.items[i].string, "", 0,
- &dir.exclude_list[EXC_CMDL]);
+ &dir.exclude_list_groups[EXC_CMDL].ary[0]);
This looks somewhat ugly for two reasons.
* The abstraction add_exclude() offers to its callers is just to
let them add one pattern to the list of patterns for the kind
(here, EXC_CMDL); why should they care about .ary[0] part? Are
there cases any sane caller (not the implementation of the
exclude_list_group machinery e.g. add_excludes_from_... function)
may want to call it with .ary[1]? I do not think of any.
Shouldn't the public API function add_exclude() take a pointer to
the list group itself?
* When naming an array of things, we tend to prefer naming it
type thing[count]
so that the second element can be called "thing[2]" and not
"things[2]". dir.exclude_list_group[EXC_CMDL] reads better.
Also, "ary[]" is a bad name, even as an implementation detail, for
two reasons: it is naming it after its type (being an "array") not
after what it is (if it holds the patterns from the same information
source, e.g. file, togeter, "src" might be a better name), and it
uses rather unusual abbreviation (I haven't seen "array" shortened
to "ary" anywhere else).
quoted
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index ef7f99a..c448e06 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -420,10 +420,10 @@ static int option_parse_z(const struct option *opt,
static int option_parse_exclude(const struct option *opt,
const char *arg, int unset)
{
- struct exclude_list *list = opt->value;
+ struct exclude_list_group *group = opt->value;
exc_given = 1;
- add_exclude(arg, "", 0, list);
+ add_exclude(arg, "", 0, &group->ary[0]);
This is another example where the caller would wish to be able to say
add_exclude(arg, "", 0, group);
instead.
On Fri, Jan 04, 2013 at 11:54:34PM -0800, Junio C Hamano wrote:
Junio C Hamano [off-list ref] writes:
quoted
Adam Spiers [off-list ref] writes:
quoted
diff --git a/builtin/clean.c b/builtin/clean.c
index 0c7b3d0..bd18b88 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -97,9 +97,10 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
if (!ignored)
setup_standard_excludes(&dir);
+ add_exclude_list(&dir, EXC_CMDL);
for (i = 0; i < exclude_list.nr; i++)
add_exclude(exclude_list.items[i].string, "", 0,
- &dir.exclude_list[EXC_CMDL]);
+ &dir.exclude_list_groups[EXC_CMDL].ary[0]);
This looks somewhat ugly for two reasons.
* The abstraction add_exclude() offers to its callers is just to
let them add one pattern to the list of patterns for the kind
(here, EXC_CMDL); why should they care about .ary[0] part? Are
there cases any sane caller (not the implementation of the
exclude_list_group machinery e.g. add_excludes_from_... function)
may want to call it with .ary[1]? I do not think of any.
Shouldn't the public API function add_exclude() take a pointer to
the list group itself?
* When naming an array of things, we tend to prefer naming it
type thing[count]
so that the second element can be called "thing[2]" and not
"things[2]". dir.exclude_list_group[EXC_CMDL] reads better.
Also, "ary[]" is a bad name, even as an implementation detail, for
two reasons: it is naming it after its type (being an "array") not
after what it is (if it holds the patterns from the same information
source, e.g. file, togeter, "src" might be a better name), and it
uses rather unusual abbreviation (I haven't seen "array" shortened
to "ary" anywhere else).
OK, well in that case Documentation/technical/api-allocation-growing.txt
needs to be fixed, because I copied it from that. I would never normally
shorten "array" to "ary" either, but I did it in an attempt to conform
to the stated guidelines.
The documentation for the ALLOC_GROW API implicitly encouraged
developers to use "ary" as the variable name for the array which is
dynamically grown. However "ary" is an unusual abbreviation hardly
used anywhere else in the source tree, and it is also better to name
variables based on their contents not on their type.
Signed-off-by: Adam Spiers <redacted>
---
Documentation/technical/api-allocation-growing.txt | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/Documentation/technical/api-allocation-growing.txt b/Documentation/technical/api-allocation-growing.txt
index 43dbe09..3894815 100644
--- a/Documentation/technical/api-allocation-growing.txt
+++ b/Documentation/technical/api-allocation-growing.txt
@@ -5,7 +5,9 @@ Dynamically growing an array using realloc() is error prone and boring.
Define your array with:
-* a pointer (`ary`) that points at the array, initialized to `NULL`;
+* a pointer (`array`) that points at the array, initialized to `NULL`
+ (although please name the variable based on its contents, not on its
+ type);
* an integer variable (`alloc`) that keeps track of how big the current
allocation is, initialized to `0`;
@@ -13,22 +15,22 @@ Define your array with:
* another integer variable (`nr`) to keep track of how many elements the
array currently has, initialized to `0`.
-Then before adding `n`th element to the array, call `ALLOC_GROW(ary, n,
+Then before adding `n`th element to the array, call `ALLOC_GROW(array, n,
alloc)`. This ensures that the array can hold at least `n` elements by
calling `realloc(3)` and adjusting `alloc` variable.
------------
-sometype *ary;
+sometype *array;
size_t nr;
size_t alloc
for (i = 0; i < nr; i++)
- if (we like ary[i] already)
+ if (we like array[i] already)
return;
/* we did not like any existing one, so add one */
-ALLOC_GROW(ary, nr + 1, alloc);
-ary[nr++] = value you like;
+ALLOC_GROW(array, nr + 1, alloc);
+array[nr++] = value you like;
------------
You are responsible for updating the `nr` variable.
--
1.7.11.7.33.gb8feba5