Good morning Stefan,
stef...@apache.org wrote on Thu, Apr 08, 2021 at 14:35:52 -:
> 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