Re: [PATCH 1/8] Refactor disassembly code

[Pedro Alves <palves@redhat.com>]

On 01/12/2017 12:19 PM, Yao Qi wrote:

>>> diff --git a/gdb/disasm.h b/gdb/disasm.h
>>> index 4c6fd54..5592cdb 100644
>>> --- a/gdb/disasm.h
>>> +++ b/gdb/disasm.h
>>> @@ -33,6 +33,48 @@ struct gdbarch;
>>> struct ui_out;
>>> struct ui_file;
>>>
>>> +class gdb_disassembler
>>> +{
>>> +  using di_read_memory_ftype = decltype
>>> (disassemble_info::read_memory_func);
>>> +
>>> +public:
>>> +  gdb_disassembler (struct gdbarch *gdbarch, struct ui_file *file)
>>> +    : gdb_disassembler (gdbarch, file, dis_asm_read_memory)
>>> +  {}
>>> +
>>> +  int print_insn (CORE_ADDR memaddr);
>>> +  int print_insn (CORE_ADDR memaddr, int *branch_delay_insns);
>>
>> Not very important, but since print_insn(CORE_ADDR) is trivial, you
>> could merge those two methods and provide a default parameter value
>> of NULL for branch_delay_insns.
> 
> OK.  Fixed them locally.

I had written it that way originally because a default parameter forces
the compiler to pass down an extra parameter (adding to register pressure)
to all call sites, when only a few places actually need the extra
output parameter.  It's like a double-optional -- i.e., the
parameter can be NULL, so merging doesn't simplify that much,
given that the version with the single argument does not need to
check the parameter.  I.e., one function can be built on top of the
other.  I see it as a different case from when a parameter is optional
such that the passed in value always need to be taken in consideration
by the method implementation, like when passing a flags argument, with
the default being some flag value (or zero).

But this is not really performance critical code, so if you
want to change it, I don't mind.

Thanks,
Pedro Alves