On Fri, 2 Sep 2016, Jakub Narębski wrote:
> W dniu 01.09.2016 o 09:49, Johannes Schindelin pisze:
> > On Wed, 31 Aug 2016, Jakub Narębski wrote:
> >> Here todo_list uses growable array implementation of list. Which is
> >> I guess better on current CPU architecture, with slow memory,
> >> limited-size caches, and adjacency prefetching.
> > That is not the reason that an array is used here. The array allows us
> > much more flexibility.
> It would be nice if this reasoning (behind the change from linked list
> to growable array) was mentioned in appropriate commit message, and
> perhaps also in the cover letter for the series. It is IMVHO quite
> important information (that you thought obvious).
> >>> +struct todo_item *append_todo(struct todo_list *todo_list)
> >> Errr... I don't quite understand the name of this function.
> >> What are you appending here to the todo_list?
> > A new item.
> >> Compare string_list_append() and string_list_append_nodup(),
> >> where the second parameter is item to append.
> > Yes, that is correct. In the case of a todo_item, things are a lot more
> > complicated, though. Some of the values have to be determined tediously
> > (such as the offset and length of the oneline after the "pick <oid>"
> > command). I just put those values directly into the newly allocated item,
> > is all.
> I would expect sth_append command to take a list (or other collection),
> an element, and return [modified] collection with the new element added.
> Such API would require temporary variable in caller and memcopy in the
> sth_append() function.
> This is not it. It creates a new element, expanding a list (a collection),
> and then expose this element. Which spares us memcopy... on non-critical
> I don't know how to name operation "grow list and return new element".
> But "append" it is not.
I renamed it to append_new_todo().
> >>> - end_of_object_name = bol + strcspn(bol, " \t\n");
> >>> + end_of_object_name = (char *) bol + strcspn(bol, " \t\n");
> >> Why is this cast needed?
> > Because bol is a "const char *" and we need to put "NUL" temporarily to
> > *end_of_object_name:
> Would compiler complain without this const'ness-stripping cast?
Yes. I would not have added it otherwise.
Please note that this is only necessary because I changed the parameter
from "char *" to "const char *" (which was The Right Thing To Do).
> >>> saved = *end_of_object_name;
> >>> *end_of_object_name = '\0';
> >>> status = get_sha1(bol, commit_sha1);
> >>> *end_of_object_name = saved;
> > Technically, this would have made a fine excuse to teach get_sha1() a
> > mode where it expects a length parameter instead of relying on a
> > NUL-terminated string.
> > Practically, such fine excuses cost me months in this rebase--helper
> > project already, and I need to protect my time better.
> Put it in TODO list (and perhaps add a TODO comment) ;-).
I am also a realist: I won't be able to do anything about this. If you
care enough, please go right to town.