Hi Kuba, 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). Amended. > >>> +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 > path. > > 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. Thanks again, Dscho
