Good morning Stefan, stef...@apache.org wrote on Thu, Apr 08, 2021 at 14:35:52 -0000: > Initial, single-theaded implementation of the svn_task__t API.
I've committed some minor fixes in r1888533. Most of them are straightforward but please double check that all the apostrophes are where they should be. Here's also a review of task.c as it stands now. Index: subversion/include/private/svn_task.h =================================================================== --- subversion/include/private/svn_task.h (revision 1888527) +++ subversion/include/private/svn_task.h (working copy) @@ -117,6 +117,9 @@ typedef svn_error_t *(*svn_task__process_func_t)( * All data that shall be returned by @a svn_task__run() needs to be * allocated from @a result_pool. * + * @note The lifetime of @a result is not guaranteed. If it is to be returned + * from svn_task__run(), it needs to be copied to @a result_pool. + * * @a cancel_func, @a cancel_baton and @a scratch_pool are the usual things. */ typedef svn_error_t *(*svn_task__output_func_t)( Index: subversion/libsvn_subr/task.c =================================================================== --- subversion/libsvn_subr/task.c (revision 1888533) +++ subversion/libsvn_subr/task.c (working copy) @@ -139,7 +139,8 @@ struct svn_task__t /* Parent task. * NULL for the root node: - * (PARENT==NULL) == (*this==ROOT->TASK). + * (PARENT==NULL) == (this==ROOT->TASK). + * ### assert() that? */ svn_task__t *parent; @@ -177,6 +178,7 @@ struct svn_task__t /* Task state. */ /* The callbacks to use. Never NULL. */ + /* ### assert() that? */ callbacks_t *callbacks; /* Process baton to pass into CALLBACKS->PROCESS_FUNC. */ @@ -186,7 +188,7 @@ struct svn_task__t * Sub-pool of ROOT->PROCESS_POOL. * * NULL, iff processing of this task has completed (sub-tasks may still - * need processing). Used to check whether processing has been completed. + * need processing). */ apr_pool_t *process_pool; @@ -203,12 +205,13 @@ struct svn_task__t /* Return the index of the first immediate sub-task of TASK with a ready * sub-task in its respective sub-tree. TASK must have at least one such - * sub-task. + * proper sub-task. */ static apr_size_t first_ready_sub_task_idx(const svn_task__t *task) { svn_task__t *sub_task = task->first_ready; assert(sub_task); + assert(sub_task != task); while (sub_task->parent != task) sub_task = sub_task->parent; @@ -391,6 +394,8 @@ static svn_error_t *remove_task(svn_task__t *task) * mainly to prevent error leaks when terminating the task runner early. * It is better to have the standard pool cleanups to deal with memory * management in that case. + * + * ### Also resets FIRST_SUB */ static void free_sub_tasks(svn_task__t *task) { @@ -503,6 +508,7 @@ static void process(svn_task__t *task, { /* Depending on whether there is prior parent output, we may or * may not have already an OUTPUT structure allocated for TASK. */ + /* ### Aside: The names could be clearer. As it is, output == task->output != output->output */ output_t *output = ensure_output(task); output->error = callbacks->process_func(&output->output, task, thread_context, @@ -555,7 +561,12 @@ static svn_error_t *output_processed( /* Post-order, i.e. dive into sub-tasks first. * * Note that the "background" processing for CURRENT has completed - * and only the output function may add no further sub-tasks. */ + * and the "foreground" output function may add further sub-tasks. */ + /* ### The 'if' branch processes current->first_sub->output->prior_parent_output — + * ### that's output produced by current — in this iteration, and only in + * ### the next iteration will handle current->first_sub's subtree's + * ### outputs. That's not post-order, is it? + */ if (current->first_sub) { current = current->first_sub; HTH, Daniel