Re: [PATCH 3/8] Disassembly unit test: disassemble one instruction

Pedro Alves
2017-01-12T15:44:49Z

On 01/12/2017 03:34 PM, Yao Qi wrote:
> On 17-01-12 15:15:34, Pedro Alves wrote:
>> On 01/10/2017 12:26 PM, Yao Qi wrote:
>>> + class gdb_disassembler_test : public gdb_disassembler
>>> + {
>>> + public:
>>> +
>>> +#ifndef DISASSEMBLER_TEST_VERBOSE
>>> + explicit gdb_disassembler_test (struct gdbarch *gdbarch,
>>> + const gdb_byte *insn)
>>> + : gdb_disassembler (gdbarch, ui_file_new (),
>>> + gdb_disassembler_test::read_memory),
>>> + m_insn (insn)
>>> + {
>>> + }
>>> +
>>> + ~gdb_disassembler_test ()
>>> + {
>>> + ui_file_delete ((struct ui_file *) m_di.stream);
>>
>>
>> Hmm, looks like you've made m_di be "protected" for
>> these uses.
>>
>> But we have the public stream() method already,
>> so I think could be:
>>
>> ~gdb_disassembler_test ()
>> {
>> ui_file_delete (stream ());
>> }
>>
>> You could then make m_di private again.
>>
>> But you shouldn't really need to create a new stream for
>> testing. We have other places that want to print to a
>> a null stream. We can factor out out the null_stream creation
>> from gdb_insn_length into a new function:
>>
>> struct ui_file *
>> null_stream ()
>> {
>> static struct ui_file *stream = NULL;
>>
>> if (stream == NULL)
>> {
>> stream = ui_file_new ();
>> make_final_cleanup (do_ui_file_delete, stream);
>> }
>> return stream;
>> }
>>
>> and then use it wherever necessary.
>>
>
> I did write code that way, but I changed it because the destroy of
> stream is done in cleanup. I want the test case depends on other
> part of GDB as less as possible. I hope this test case can be run
> even without cleanup stuff. It is sort of RAII (stream is regarded
> as resource).

Note that's a _final_ cleanup. I.e., it only runs when GDB exits.
There's no need for gdb_disassembler_test to destroy the stream
with that.

And in my palves/cxx-eliminate-cleanups branch, where I've
C++-fied the whole ui_file hierarchy, the cleanup completely
disappears.

Thanks,
Pedro Alves