[RFC] New macro SVN_WC__DB_WITH_TXN(my_func_call(...)) doesn't need a baton

[Julian Foad <ju...@btopenworld.com>]

The "call a function within a transaction" paradigm is used heavily in wc_db.c.  The current implementation uses a callback function with a standard signature, passing all other parameters to it via a "baton":

  struct base_remove_baton {
    param1;
    param2;
    ...
  };

  /* ...
     Implements svn_wc__db_txn_callback_t. */
  static svn_error_t *db_base_remove(void *baton,
                                     wcroot,
                                     local_relpath,
                                     scratch_pool)
  {
    struct base_remove_baton *rb = baton;

    ... rb->param1 ...
  }

  In the caller:
  {
    struct base_remove_baton rb;

    rb.param1 = param1;
    rb.param2 = param2;
    ...
    SVN_ERR(svn_wc__db_with_txn(wcroot, local_relpath, db_base_remove,
                                &rb, scratch_pool));
  }

But this is unnecessarily verbose.  To simplify it, we can write a statement macro that takes a complete, arbitrary function call as an argument.  Pseudo-code (omitting error handling):

#define WITH_TXN(db, wrapped_function_call)
  {
    start_txn(db)
    if (wrapped_function_call) succeeds:
      commit_txn(db)
    else:

      roll_back_txn(db)
  }

(The argument need not be a function call, it can be any expression of type svn_error_t*.)

Then the usage looks like this:

  /* ... */
  static svn_error_t *db_base_remove(param1,
                                     param2,
                                     ...,
                                     scratch_pool)
  {
    ... param1 ...
  }

  In the caller:
  {
    SVN_WC__DB_WITH_TXN(wcroot,
      db_base_remove(param1, param2, ..., scratch_pool));
  }


This style can be used not only for DB transactions but anywhere we need to wrap a call inside a resource acquisition and release.  We already use this style for working with mutex locks:

  SVN_MUTEX__WITH_LOCK(mutex, expr)

And this style could also be used in place of:

  with_txnlist_lock() in fs_fs.c

and probably others.

  

The main advantage is:

  * Simpler usage.

Potential down-sides:

 
 * Macros can be harder to debug in some environments?  But we already 
accept that svn_wc__db_with_txn is wrapped in SVN_ERR so I don't see a 
problem.

The attached patch contains an implementation and some examples of usage.  I would tweak the macro definition and support functions a bit, and there is a bug in that patch somewhere, but you get the idea.

I would prefer to be working with the simpler style of code, so I would like to introduce and use this macro.

What does anybody think of introducing this form, in principle and in practice?

- Julian



--
Certified & Supported Apache Subversion Downloads: http://www.wandisco.com/subversion/download

Re: [RFC] New macro SVN_WC__DB_WITH_TXN(my_func_call(...)) doesn't need a baton

[Julian Foad <ju...@btopenworld.com>]

I (Julian Foad) wrote:

> The "call a function within a transaction" paradigm is used heavily in 
> wc_db.c.  [...]  To simplify it, we can write a statement 
> macro that takes a complete, arbitrary function call as an argument.  
> [...] Then the usage looks like this:
> 
>   /* ... */
>   static svn_error_t *db_base_remove(param1,
>                                      param2,
>                                      ...,
>                                      scratch_pool)
>   {
>     ... param1 ...
>   }
> 
>   In the caller:
>   {
>     SVN_WC__DB_WITH_TXN(wcroot,
>       db_base_remove(param1, param2, ..., scratch_pool));
>   }
[...]
> I would prefer to be working with the simpler style of code, so I would like to 
> introduce and use this macro.

I will point out that there are also some places where the current use of a baton still makes sense, notably for these three functions:

  insert_base_node()
  insert_external_node()
  insert_working_node()

where the baton contains a set of logically related information about the node rather than things that would make sense to be function parameters.  Using the new macro for these while keeping the baton will enable type checking of the baton type but nothing major.

> What does anybody think of introducing this form, in principle and in practice?

If there are no objections in a day or so, I'll finish the patch and commit it.

- Julian