[petsc-dev] inconsistency for its own sake?

[Barry Smith]

On Jan 11, 2013, at 12:12 PM, Jed Brown <jedbrown at mcs.anl.gov> wrote:

> Ah, Dependency versus Duplication. I blame the lack of transparency on the editors, which could display the definition of the typedef instead of having to jump to it. I like that the typedefs allow us to document the function in one place and that changing arguments (hopefully only while working out the initial implementation, but whenever it's deemed necessary) is more reliable. Since dynamic functions are cast to void(*)(void), the compiler may not be able to check that types match. Also, ability to grep for any place that a given callback is manipulated.
> 
> Pros of typedef:
> * typedef is, in principle, "safer" for developers
> * typedef helps avoid duplication in documentation; maintains symmetry between Get() and Set()
> * typedef uses simpler type notation (nice for less experienced C programmers)
> 

    I will give more information about my plan that removes some (but not all) of your concerns

/*M 
     SNESFunction - function used to convey the nonlinear function to be solved by SNES

     Calling sequence of func

   SNESFunction(SNES snes,Vec x,Vec f,void *ctx);

snes	- the SNES context
x	- state at which to evaluate residual
f	- vector to put residual
ctx	- optional user-defined function context

.seealso:   SNESSetFunction()
M*/

#undef __FUNCT__
#define __FUNCT__ "SNESSetFunction"
/*@C
   SNESSetFunction - Sets the function evaluation routine and function
   vector for use by the SNES routines in solving systems of nonlinear
   equations.

   Logically Collective on SNES

   Input Parameters:
+  snes - the SNES context
.  r - vector to store function value
.  SNESFunction - function evaluation routine
-  ctx - [optional] user-defined context for private data for the
         function evaluation routine (may be PETSC_NULL)

   Notes:
   The Newton-like methods typically solve linear systems of the form
$      f'(x) x = -f(x),
   where f'(x) denotes the Jacobian matrix and f(x) is the function.

   Level: beginner

.keywords: SNES, nonlinear, set, function

.seealso: SNESFunction, SNESGetFunction(), SNESComputeFunction(), SNESSetJacobian(), SNESSetPicard()
@*/
PetscErrorCode  SNESSetFunction(SNES snes,Vec r,PetscErrorCode (*SNESFunction)(SNES,Vec,Vec,void*),void *ctx)
{

Note that since SNESFunction has a manual page the user can click on it to see the details of the calling sequence arguments. Thus each manual page that uses SNESFunction() doesn't have to duplicate the details of the arguments to SNESFunction(). Also search and replace on SNESFunction can be used to make changes (yes, I admit not exactly as easy as with a typedef but still at least can be done systematically).

Is this a good middle-ground? Something wrong with it?

  Barry

> Cons of typedef:
> * typedef is less transparent when reading code
> * information is more scattered (harder for people that don't have the source code indexed and don't know where to find definitions)
> * because every usage does not need to be updated, it's hard to determine consequences of a change when reading diff
> 
> I'll stop using function pointer typedefs if you don't like them.
> 
> On Fri, Jan 11, 2013 at 10:26 AM, Barry Smith <bsmith at mcs.anl.gov> wrote:
> 
>    The thing I don't like about typedef's for function pointers is that they do not display the calling sequencing in the code
> 
>   For example  from
> 
> > typedef struct _n_TSDM *TSDM;
> > struct _n_TSDM {
> >   TSRHSFunction rhsfunction;
> 
>   or
> 
>    PetscErrorCode TSSetRHSFunction(TS ts, TSRHSFunction rhsfunction)
> 
>   I have no freaking idea what the calling sequence of rhsfunction is. but with
> 
> > typedef struct _n_TSDM *TSDM;
> > struct _n_TSDM {
> >   PetscErrorCode (*TSRHSFunction)(TS,PetscReal,Vec,Vec,void*);
> 
>  or
> 
>     PetscErrorCode TSSetRHSFunction(TS ts, PetscErrorCode (*TSRHSFunction)(TS,PetscReal,Vec,Vec,void*);
> 
> boom I know exactly the calling sequence.
> 
>    Maybe there is some way we can do manual pages for these beasties and consistency of names in different locations so we get the best of both worlds?
> 
>    Barry
> 
> 
> 
> On Nov 25, 2012, at 3:58 AM, Jed Brown <jedbrown at mcs.anl.gov> wrote:
> 
> > On Sun, Nov 25, 2012 at 4:42 AM, Barry Smith <bsmith at mcs.anl.gov> wrote:
> >
> > typedef struct _n_KSPDM *KSPDM;
> > struct _n_KSPDM {
> >   PetscErrorCode (*computeoperators)(KSP,Mat,Mat,MatStructure*,void*);
> >   PetscErrorCode (*computerhs)(KSP,Vec,void*);
> >   PetscErrorCode (*computeinitialguess)(KSP,Vec,void*);
> >
> > typedef struct _n_SNESDM *SNESDM;
> > struct _n_SNESDM {
> >   PetscErrorCode (*computefunction)(SNES,Vec,Vec,void*);
> >   PetscErrorCode (*computegs)(SNES,Vec,Vec,void*);
> >   PetscErrorCode (*computejacobian)(SNES,Vec,Mat*,Mat*,MatStructure*,void*);
> >
> >   /* objective */
> >   PetscErrorCode (*computeobjective)(SNES,Vec,PetscReal*,void*);
> >
> >
> > typedef struct _n_TSDM *TSDM;
> > struct _n_TSDM {
> >   TSRHSFunction rhsfunction;
> >   TSRHSJacobian rhsjacobian;
> >
> >   TSIFunction ifunction;
> >   TSIJacobian ijacobian;
> >
> > The PETSc style guide says  to avoid typedef of function pointers unless there is a damn good reason to use them (or it should);
> >
> > Can we revisit this choice in the interest of writing one man page that documents the assumptions that one can make about a callback routine? I hate duplicating the same information in TS{Set,Get}IFunction() and DMTS{Set,Get}IFunction(). Also, if a typedef is used everywhere, we can more reliably find all places that depend on it. (using grep or M-x gtags-find-symbol).
> >
> > but why use them for TS but not for SNES or KSP functions?
> >
> > Because they were already defined for use with TSSetIFunction()/TSGetIFunction().
> 
>