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

Tom Tromey
2013-03-13T19:26:47Z

>>>>> "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