Re: [PATCH v3 01/15] Refactor 'tsave'

[Tom Tromey <tromey@redhat.com>]

>>>>> "Yao" == Yao Qi <yao@codesourcery.com> writes:

Tom> So, I think it would be better to have an explicit "dtor" method that is
Tom> called to clean up.

Yao> I Agree.  A new field <dtor> is added in 'struct trace_file_write_ops',
Yao> which is responsible for release file descriptor and memory.  The
Yao> <dtor> will be called in trace_file_writer_xfree.

Thanks.

Tom> It is strange to see multiple virtual calls in a row like this.

Tom> Is there some reason not to collapse them into a single call?

Yao> It is useful to reduce the complexity of each hook function of 'struct
Yao> trace_file_write_ops'.  Each function will be small and clear.

Nothing constrains the implementation to be one huge function.
But, that said, this is fine as is.

Yao> +static void
Yao> +tfile_dtor (struct trace_file_writer *self)
Yao> +{
Yao> +  struct tfile_trace_file_writer *writer
Yao> +    = (struct tfile_trace_file_writer *) self;
Yao> +
Yao> +  xfree (writer->pathname);
Yao> +  fclose (writer->fp);

I think this needs a NULL check.
Otherwise I think on error this may crash.

Ok with that fixed.

Tom