| Message ID | 1490605592-12189-5-git-send-email-jtotto@uwaterloo.ca (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@uwaterloo.ca> -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. 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. ~Andrew
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@uwaterloo.ca> > > -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
diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c index 61fc4a4..86f6903 100644 --- a/tools/libxc/xc_sr_save.c +++ b/tools/libxc/xc_sr_save.c @@ -47,24 +47,18 @@ static int write_headers(struct xc_sr_context *ctx, uint16_t guest_type) } /* - * Writes an END record into the stream. + * Declares a helper function to write an empty record of a particular type. */ -static int write_end_record(struct xc_sr_context *ctx) -{ - struct xc_sr_record end = { REC_TYPE_END, 0, NULL }; - - return write_record(ctx, ctx->fd, &end); -} - -/* - * Writes a CHECKPOINT record into the stream. - */ -static int write_checkpoint_record(struct xc_sr_context *ctx) -{ - struct xc_sr_record checkpoint = { REC_TYPE_CHECKPOINT, 0, NULL }; +#define WRITE_TRIVIAL_RECORD_FN(name, type) \ + static int write_ ## name ## _record(struct xc_sr_context *ctx) \ + { \ + struct xc_sr_record name = { (type), 0, NULL }; \ + \ + return write_record(ctx, ctx->fd, &name); \ + } - return write_record(ctx, ctx->fd, &checkpoint); -} +WRITE_TRIVIAL_RECORD_FN(end, REC_TYPE_END); +WRITE_TRIVIAL_RECORD_FN(checkpoint, REC_TYPE_CHECKPOINT); /* * Writes a batch of memory as a PAGE_DATA record into the stream. The batch
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@uwaterloo.ca> --- tools/libxc/xc_sr_save.c | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-)