Re: [PATCH 1/2] Add unit test to aarch64 prologue analyzer

[Antoine Tremblay <antoine dot tremblay at ericsson dot com>]

Yao Qi writes:

> On Tue, Nov 29, 2016 at 09:57:46AM -0500, Antoine Tremblay wrote:
>> 
>> Yao Qi writes:
>> 
>> > @@ -436,6 +461,89 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
>> >    return start;
>> >  }
>> >  
>> > +static CORE_ADDR
>> > +aarch64_analyze_prologue (struct gdbarch *gdbarch,
>> > +			  CORE_ADDR start, CORE_ADDR limit,
>> > +			  struct aarch64_prologue_cache *cache)
>> > +{
>> > +  instruction_reader reader { };
>> > +
>> 
>> Could we use the default constructor here? If it's kept.
>> 
>
> It uses the default constructor.  instruction_reader has the default
> constructor.
>
> + public:
> +  instruction_reader () = default;
>
> Do you mean I need to remove it?
>

No, sorry I need to improve on C++11 I thought you needed a
std::initializer_list to init with {} like in the _test class but it
it works with the default constructor.

>> > +  return aarch64_analyze_prologue (gdbarch, start, limit, cache,
>> > +				   reader);
>> > +}
>> > +
>> > +#if GDB_SELF_TEST
>> > +
>> > +namespace selftests {
>> > +
>> > +  /* Instruction reader from manually cooked instruction sequences.  */
>> > +  class instruction_reader_test : public abstract_instruction_reader
>> > +  {
>> > +  public:
>> > +    instruction_reader_test() = default ;
>> 
>> Very nit, but there's a space before ';'
>> 
>
> Oh, yes, it should be "instruction_reader_test () = default;"
>
>> Also I wonder if we need to specify the default constructor explicitly ?
>> Is there a rationale for it?
>> 
>> It's never used too, unless you apply my previous comment.
>
> The instruction_reader_test default constructor is never used.  How
> about using "= delete"?
>
>     instruction_reader_test () = delete;
>     instruction_reader_test (std::initializer_list<uint32_t> init)
>       : insns{init} {}

Yes that would be more appropriate if we're going to specify that.

I just wrote a patch with a C++ class and did not include explicit
default constructors do you think we should make it a code convention to
explicitly specify their existence or non-existence (=default, =delete) ?

I could not find mention of that in GCC's C++ conventions...