While writing the bulk-move series, I introduced 2 new functions,
locate_bulkmove_candidate() and locate_rename_dst_dir(), closely
derived from locate_rename_dst(). That was because it seemed not so
trivial to overcome the differences in those 3 funcs while retaining
readability.
Now when working on bulk-rm (as a new basis for next-gen bulk-move), I
find myself in the need of adding more of them: again functions for
searching/inserting in a sorted list of various contents. My first
approach (this series) was to generalize locate_rename_dst() to
operate on other "lists of a filespec and something", since that was
matching my needs for "bulkrm_candidates".
Tell me your opinion on this: I find the result hard to read, and
difficult to adapt to all of locate_rename_dst_dir() with its
non-filespec input, let alone locate_bulkmove_candidate() with 2 of
them. And even the memset() call is probably less efficient than the
original "= NULL".
So I'm planning of redoing the thing with a different principle:
change the core function to accept func-pointer for comparison and
initialization - that would make the core func much more readable.
Since those trivial funcs would be trivial static ones, compilers
ought to be able to instantiate specific versions of the core func,
inlining those trivial ones; if it does not, however, that would
surely give a performance penalty if it does not...
The alternative, having several mostly-identical funcs, which have to
be made generic anyway to prevent proliferation, seems to me much more
of a hell. What's the general opinion ?
@@ -37,15 +44,15 @@ static struct diff_rename_dst *locate_rename_dst(struct diff_filespec *two,if(!insert_ok)returnNULL;/* insert to make it at "first" */-if(rename_dst_alloc<=rename_dst_nr){-rename_dst_alloc=alloc_nr(rename_dst_alloc);+if((*elem_alloc_p)<=(*elem_nr_p)){+(*elem_alloc_p)=alloc_nr(*elem_alloc_p);rename_dst=xrealloc(rename_dst,-rename_dst_alloc*sizeof(*rename_dst));+(*elem_alloc_p)*sizeof(*rename_dst));}-rename_dst_nr++;-if(first<rename_dst_nr)+(*elem_nr_p)++;+if(first<(*elem_nr_p))memmove(rename_dst+first+1,rename_dst+first,-(rename_dst_nr-first-1)*sizeof(*rename_dst));+((*elem_nr_p)-first-1)*sizeof(*rename_dst));rename_dst[first].two=alloc_filespec(two->path);fill_filespec(rename_dst[first].two,two->sha1,two->mode);rename_dst[first].pair=NULL;
Now offset calculations don't rely on pointer arithmetic any more, paving
the way for complete decoupling from diff_rename_dst.
---
diffcore-rename.c | 23 +++++++++++++----------
1 files changed, 13 insertions(+), 10 deletions(-)
The dst_obj struct acts as an absract base class for the elements which
_locate_element() manipulates arrays of.
---
diffcore-rename.c | 21 +++++++++++++--------
1 files changed, 13 insertions(+), 8 deletions(-)
@@ -51,13 +51,12 @@ static void *_locate_element(struct diff_filespec *two,/* insert to make it at "first" */if((*elem_alloc_p)<=(*elem_nr_p)){(*elem_alloc_p)=alloc_nr(*elem_alloc_p);-rename_dst=xrealloc(rename_dst,-(*elem_alloc_p)*elem_size);+*listp=xrealloc(*listp,(*elem_alloc_p)*elem_size);}(*elem_nr_p)++;-first_elem_p=(void*)rename_dst+first*elem_size;+first_elem_p=*listp+first*elem_size;if(first<(*elem_nr_p))-memmove((void*)rename_dst+(first+1)*elem_size,+memmove(*listp+(first+1)*elem_size,first_elem_p,((*elem_nr_p)-first-1)*elem_size);first_elem_p->two=alloc_filespec(two->path);
Is this syntactic sugar needed?
static struct diff_rename_dst *locate_rename_dst(struct diff_filespec *two,
insert_ok)
{
return locate_element(&rename_dst_nr, &rename_dst_alloc, elem, insert_ok);
}
takes more advantage of the compiler's typechecking and looks easy
enough to read.
Since this is local to diffcore-rename, I don't mind the locate_element()
name, but if this is to be used more widely I think it would need to be
named more precisely. (find_or_insert_in_array()?)
-static struct diff_rename_dst *locate_rename_dst(struct diff_filespec *two,
- int insert_ok)
+static struct diff_rename_dst *_locate_element(struct diff_filespec *two,
+ int *elem_nr_p, int *elem_alloc_p,
+ int insert_ok)
{
int first, last;
first = 0;
- last = rename_dst_nr;
+ last = (*elem_nr_p);
I guess these parentheses came from search+replace? It's more
readable without them.
[...]
+ (*elem_nr_p)++;
Except for this one.
Generally, the approach seems sane so far.
The idea was to deprecate locate_rename_dst() itself and only use
locate_element()...
takes more advantage of the compiler's typechecking and looks easy
enough to read.
... but typechecking was something that was worrying me here. Looks
like a good idea, after all.
Since this is local to diffcore-rename, I don't mind the locate_element()
name, but if this is to be used more widely I think it would need to be
named more precisely. (find_or_insert_in_array()?)
Yes - that's just a prototype.
I guess these parentheses came from search+replace? It's more
readable without them.