Re: [Xen-devel] [PATCH RFC 04/20] libxc/xc_sr_save.c: add WRITE_TRIVIAL_RECORD_FN()

[Joshua Otto <jtotto@xxxxxxxxxxxx>]

On Tue, Mar 28, 2017 at 08:03:26PM +0100, Andrew Cooper wrote:
> On 27/03/17 10:06, Joshua Otto wrote:
> > Writing the libxc save stream requires writing a few 'trivial' records,
> > consisting only of a header with a particular type.  As a readability
> > aid, it's nice to have obviously-named functions that write these sorts
> > of records into the stream - for example, the first such function was
> > write_end_record(), which reads much more pleasantly at its call-site
> > than write_generic_record(REC_TYPE_END) would.  However, it's tedious
> > and error-prone to copy-paste the generic body of such a function for
> > each new trivial record type.
> >
> > Add a helper macro that takes a name base and a record type and declares
> > the corresponding trivial record write function.  Use this to re-define
> > the two existing trivial record functions, write_end_record() and
> > write_checkpoint_record().
> >
> > No functional change.
> >
> > Signed-off-by: Joshua Otto <jtotto@xxxxxxxxxxxx>
> 
> -1.
> 
> This hides the functions from tools like cscope, and makes the code
> harder to read.  I also don't really buy the error prone argument.

Okay, fair enough.

> 
> If you do want to avoid opencoding different functions, how about
> 
> static int write_zerolength_record(uint32_t record_type)
> 
> and updating the existing callsites to be
> 
> write_zerolength_record(REC_TYPE_END); etc.

I really do prefer write_end_record() to write_some_record(REC_TYPE_END),
visually.  I'll fix up the later patches to add the corresponding functions
without the macro.

Josh

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel