Re: [Xen-devel] [PATCH 14/15] libxl: New API for providing OS events to libxl

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 14/15] libxl: New API for 
providing OS events to libxl"):
> On Fri, 9 Dec 2011, Ian Jackson wrote:
> > The alternative, repeating the declaration, violates the principle
> > that things should be said (and defined) in the code if that's
> > possible, rather than having the primary reference be a comment.  It
> > also violates the principle of trying to avoid putting the same
> > information in two places.
> 
> I would prefer having a brief explanation of what the parameters are
> before the function.  At least a few times this what my eyes where
> looking for reading this patch.

The parameters are listed in the function prototype!  That's what the
function prototype is!  The very first thing you should be reading is
the function prototype.

If by "what they are" you mean you want to know what they mean, then
that is why we give them names rather than just using anonymous
parameters.

Or are you saying that you want something like this:

  int (*fd_register)(void *user, int fd, void **for_app_registration_out,
                     short events, void *for_libxl);
    /* @fd@  the file descriptor
     ...

This is pointless boilerplate.  Obviously "int fd" is a file
descriptor and it doesn't help to write it out in English as well
as C.

This style also encourages formulaic descriptions like this:
     * @reg@  application's registration token (out parameter)

> See for example arch/x86/include/asm/paravirt_types.h in the linux
> kernel: the long description of the MACROs is before the MACROs.

Macros are different because they don't have prototypes, just
definitions.  Nontrivial macros often need a comment beforehand to
serve in place of the function prototype.

You'll see that in general my macros have the comment beforehand, for
example:

   /*
    * Inserts "elm_new" into the sorted list "head".
    *
    * "elm_search" must be a loop search variable of the same type as
    * "elm_new".  "new_after_search_p" must be an expression which is
    * true iff the element "elm_new" sorts after the element
    * "elm_search".
    *
    * "search_body" can be empty, or some declaration(s) and statement(s)
    * needed for "new_after_search_p".
    */
   #define LIBXL_TAILQ_INSERT_SORTED(head, entry, elm_new, elm_search,     \
                                     search_body, new_after_search_p)      \
       do {                                                                \


> > > > +typedef struct libxl_osevent_hooks {
> > > > +  int (*fd_register)(void *uselibxl_event_check.r, int fd, void 
> > > > **for_app_registration_out,
> > > > +                     short events, void *for_libxl);
> > > > +  int (*fd_modify)(void *user, int fd, void 
> > > > **for_app_registration_update,
> > > > +                   short events);
> > > > +  void (*fd_deregister)(void *user, int fd, void 
> > > > *for_app_registration);
> > > > +  int (*timeout_register)(void *user, void **for_app_registration_out,
> > > > +                          struct timeval abs, void *for_libxl);
> > > > +  int (*timeout_modify)(void *user, void **for_app_registration_update,
> > > > +                         struct timeval abs);
> > > > +  void (*timeout_deregister)(void *user, void 
> > > > *for_app_registration_io);
> > > > +} libxl_osevent_hooks;
> > > > +
> > > > +void libxl_osevent_register_hooks(libxl_ctx *ctx,
> > > > +                                  const libxl_osevent_hooks *hooks,
> > > > +                                  void *user);
> > ...
> > > while this description is very verbose, it doesn't contain informations
> > > on:
> > > 
> > > - what is void *user;
> > 
> > I would have thought this was obvious.  Every situation in C where a
> > function pointer is passed needs also to pass a void* (or the
> > equivalent) so that some context or dynamic information from the
> > original caller can be passed to the inner called function.  So user
> > is stored by libxl and passed to the hooks.
> 
> It might be obvious but it is not written anywhere. Considering the
> level of details you went through in the following paragraph, I am
> surprised that you left to guessing what this parameter is for. Better
> be pedantic than leaving things to imagination.

All of the details in that paragraph are there because they not
otherwise clear.  In particular, there are many semantic details which
need to be covered.  It's precisely because there are so many
important details to state, that it's not helpful to restate in
sentences in comments things which have already been stated in the
choice of function and parameter names.

But I can add a sentence about "user" if that would help:

   *
   * The value of user is stored by libxl and passed to the callbacks.

Would that address this objection ?

I definitely don't want to add a sentence next to "timeout_register"
saying "this is for libxl to register a timeout" and another sentence
saying that the "struct timeval abs" is the absolute time at which the
timeout should fire and another sentence saying that "int fd" is the
file descriptor and on on.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel