[PATCH] gdb: Use vector::emplace_back

[PATCH] gdb: Use vector::emplace_back

[Pedro Alves]

Now that we require C++11, we can use vector::emplace_back to
construct elements in place instead of constructing and then copying.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* main.c (struct cmdarg): Add constructor.
	(captured_main_1): Use vector::emplace_back.
	* tracepoint.c (collection_list::add_memrange): Likewise.
---
 gdb/main.c       | 31 ++++++++++---------------------
 gdb/tracepoint.c |  2 +-
 2 files changed, 11 insertions(+), 22 deletions(-)

diff --git a/gdb/main.c b/gdb/main.c
index ca6a81e..fe57bf8 100644
--- a/gdb/main.c
+++ b/gdb/main.c
@@ -444,7 +444,12 @@ enum cmdarg_kind
 };
 
 /* Arguments of --command option and its counterpart.  */
-struct cmdarg {
+struct cmdarg
+{
+  cmdarg (cmdarg_kind type_, char *string_)
+    : type (type_), string (string_)
+  {}
+
   /* Type of this option.  */
   enum cmdarg_kind type;
 
@@ -745,32 +750,16 @@ captured_main_1 (struct captured_main_args *context)
 	    pidarg = optarg;
 	    break;
 	  case 'x':
-	    {
-	      struct cmdarg cmdarg = { CMDARG_FILE, optarg };
-
-	      cmdarg_vec.push_back (cmdarg);
-	    }
+	    cmdarg_vec.emplace_back (CMDARG_FILE, optarg);
 	    break;
 	  case 'X':
-	    {
-	      struct cmdarg cmdarg = { CMDARG_COMMAND, optarg };
-
-	      cmdarg_vec.push_back (cmdarg);
-	    }
+	    cmdarg_vec.emplace_back (CMDARG_COMMAND, optarg);
 	    break;
 	  case OPT_IX:
-	    {
-	      struct cmdarg cmdarg = { CMDARG_INIT_FILE, optarg };
-
-	      cmdarg_vec.push_back (cmdarg);
-	    }
+	    cmdarg_vec.emplace_back (CMDARG_INIT_FILE, optarg);
 	    break;
 	  case OPT_IEX:
-	    {
-	      struct cmdarg cmdarg = { CMDARG_INIT_COMMAND, optarg };
-
-	      cmdarg_vec.push_back (cmdarg);
-	    }
+	    cmdarg_vec.emplace_back (CMDARG_INIT_COMMAND, optarg);
 	    break;
 	  case 'B':
 	    batch_flag = batch_silent = 1;
diff --git a/gdb/tracepoint.c b/gdb/tracepoint.c
index 3d28606..2a40b16 100644
--- a/gdb/tracepoint.c
+++ b/gdb/tracepoint.c
@@ -919,7 +919,7 @@ collection_list::add_memrange (int type, bfd_signed_vma base,
   /* type: memrange_absolute == memory, other n == basereg */
   /* base: addr if memory, offset if reg relative.  */
   /* len: we actually save end (base + len) for convenience */
-  m_memranges.push_back (memrange (type, base, base + len));
+  m_memranges.emplace_back (type, base, base + len);
 
   if (type != memrange_absolute)    /* Better collect the base register!  */
     add_register (type);
-- 
2.5.5

Re: [PATCH] gdb: Use vector::emplace_back

[Yao Qi]

Hi Pedro,
Patch is good to me, a question on coding style below,

On Wed, Nov 9, 2016 at 12:39 AM, Pedro Alves <palves@redhat.com> wrote:
>  /* Arguments of --command option and its counterpart.  */
> -struct cmdarg {
> +struct cmdarg
> +{
> +  cmdarg (cmdarg_kind type_, char *string_)
> +    : type (type_), string (string_)
> +  {}
> +

Is there any reason you name parameters with tailing "_"?  I don't
see anything about tailing "_" in  GDB or GCC C++ code standard.

-- 
Yao (齐尧)

Re: [PATCH] gdb: Use vector::emplace_back

[Pedro Alves]

On 11/09/2016 11:12 AM, Yao Qi wrote:
> Hi Pedro,
> Patch is good to me, a question on coding style below,
> 
> On Wed, Nov 9, 2016 at 12:39 AM, Pedro Alves <palves@redhat.com> wrote:
>>  /* Arguments of --command option and its counterpart.  */
>> -struct cmdarg {
>> +struct cmdarg
>> +{
>> +  cmdarg (cmdarg_kind type_, char *string_)
>> +    : type (type_), string (string_)
>> +  {}
>> +
> 
> Is there any reason you name parameters with tailing "_"?  I don't
> see anything about tailing "_" in  GDB or GCC C++ code standard.

We just need names for the parameters that are obviously similar
to the public members of the struct they'll be assigned to
in the member initializer list just below.  (We use "m_" prefix for
private members.  Public members of plain old data structs don't get
the "m_" prefix.)

Leading or trailing underscore are the most obvious choices, I think.

I mildly prefer trailing over leading for being less easily confused
with "m_" IMO, and also, some coding conventions use single leading
underscore for private member.  I see gcc using trailing underscore
for "shadow" parameters too.  E.g.:

id_base::id_base (id_kind kind_, const char *id_, int nargs_)
{
  kind = kind_;
  id = id_;
  nargs = nargs_;
  hashval = htab_hash_string (id);
}

But it's probably possible to find different examples if you
look deep enough.

Would you do/prefer something different?

Thanks,
Pedro Alves

Re: [PATCH] gdb: Use vector::emplace_back

[Yao Qi]

On Wed, Nov 9, 2016 at 12:21 PM, Pedro Alves <palves@redhat.com> wrote:
>
> We just need names for the parameters that are obviously similar
> to the public members of the struct they'll be assigned to
> in the member initializer list just below.  (We use "m_" prefix for
> private members.  Public members of plain old data structs don't get
> the "m_" prefix.)
>
> Leading or trailing underscore are the most obvious choices, I think.

I know leading underscore is used in some projects, so I want to know
is it a C++ code standard that we use trailing underscore in this case or
it is your personal coding habit.  It is the latter.

>
> I mildly prefer trailing over leading for being less easily confused
> with "m_" IMO, and also, some coding conventions use single leading
> underscore for private member.  I see gcc using trailing underscore
> for "shadow" parameters too.  E.g.:
>
> id_base::id_base (id_kind kind_, const char *id_, int nargs_)
> {
>   kind = kind_;
>   id = id_;
>   nargs = nargs_;
>   hashval = htab_hash_string (id);
> }
>
> But it's probably possible to find different examples if you
> look deep enough.
>
> Would you do/prefer something different?
>

Since the trailing underscore usage like this is not mentioned in C++
code standard, people are free to use or not to use it.  I don't have
a preference on that.

-- 
Yao (齐尧)

Re: [PATCH] gdb: Use vector::emplace_back

[Pedro Alves]

On 11/09/2016 12:42 PM, Yao Qi wrote:

> I know leading underscore is used in some projects, so I want to know
> is it a C++ code standard that we use trailing underscore in this case or
> it is your personal coding habit.  It is the latter.

...

> Since the trailing underscore usage like this is not mentioned in C++
> code standard, people are free to use or not to use it.  I don't have
> a preference on that.

OK.  I may ask a couple gcc people for their preference and see about
adding it to the docs.  Each detail in the standard is based on 
someone's personal preference that had sufficient following/agreement,
after all.  :-)

Thanks,
Pedro Alves

Re: [PATCH] gdb: Use vector::emplace_back

[John Baldwin]

On Wednesday, November 09, 2016 12:55:52 PM Pedro Alves wrote:
> On 11/09/2016 12:42 PM, Yao Qi wrote:
> 
> > I know leading underscore is used in some projects, so I want to know
> > is it a C++ code standard that we use trailing underscore in this case or
> > it is your personal coding habit.  It is the latter.
> 
> ...
> 
> > Since the trailing underscore usage like this is not mentioned in C++
> > code standard, people are free to use or not to use it.  I don't have
> > a preference on that.
> 
> OK.  I may ask a couple gcc people for their preference and see about
> adding it to the docs.  Each detail in the standard is based on 
> someone's personal preference that had sufficient following/agreement,
> after all.  :-)

If the goal is to support -Wshadow then it would be nice to settle on a style
so it is consistent across the tree.

-- 
John Baldwin

Re: [PATCH] gdb: Use vector::emplace_back

[Pedro Alves]

On 11/09/2016 02:48 PM, John Baldwin wrote:
> On Wednesday, November 09, 2016 12:55:52 PM Pedro Alves wrote:
>> On 11/09/2016 12:42 PM, Yao Qi wrote:
>>
>>> I know leading underscore is used in some projects, so I want to know
>>> is it a C++ code standard that we use trailing underscore in this case or
>>> it is your personal coding habit.  It is the latter.
>>
>> ...
>>
>>> Since the trailing underscore usage like this is not mentioned in C++
>>> code standard, people are free to use or not to use it.  I don't have
>>> a preference on that.
>>
>> OK.  I may ask a couple gcc people for their preference and see about
>> adding it to the docs.  Each detail in the standard is based on 
>> someone's personal preference that had sufficient following/agreement,
>> after all.  :-)
> 
> If the goal is to support -Wshadow then it would be nice to settle on a style
> so it is consistent across the tree.

It's not really about -Wshadow.  In C++, in order to use a
member initializer, like in: 

  cmdarg (cmdarg_kind type_, char *string_)
    : type (type_), string (string_)
  {}

The parameter names really must be different from the
struct's elements.

This:

  cmdarg (cmdarg_kind type, char *string)
    : this->type (type), this->string (string)
  {}

is not valid C++ and does _not_ compile:

src/gdb/main.c:450:7: error: expected identifier before ‘this’
     : this->type (type_), this->string (string_)
       ^

This instead would work:

  cmdarg (cmdarg_kind type, char *string)
  {
    this->type = type;
    this->string = string;
  }

However, using member initializer lists is a better
default, because there are cases where the above using
assignment wouldn't work or wouldn't be as efficient.  E.g., in
case the element being constructed has a heavy constructor, does
not have a default constructor at all, or doesn't have an
assignment operator.

You can find more here:

 http://stackoverflow.com/questions/926752/why-should-i-prefer-to-use-member-initialization-list

Thanks,
Pedro Alves

Re: [PATCH] gdb: Use vector::emplace_back

[John Baldwin]

On Wednesday, November 09, 2016 03:27:05 PM Pedro Alves wrote:
> On 11/09/2016 02:48 PM, John Baldwin wrote:
> > On Wednesday, November 09, 2016 12:55:52 PM Pedro Alves wrote:
> >> On 11/09/2016 12:42 PM, Yao Qi wrote:
> >>
> >>> I know leading underscore is used in some projects, so I want to know
> >>> is it a C++ code standard that we use trailing underscore in this case or
> >>> it is your personal coding habit.  It is the latter.
> >>
> >> ...
> >>
> >>> Since the trailing underscore usage like this is not mentioned in C++
> >>> code standard, people are free to use or not to use it.  I don't have
> >>> a preference on that.
> >>
> >> OK.  I may ask a couple gcc people for their preference and see about
> >> adding it to the docs.  Each detail in the standard is based on 
> >> someone's personal preference that had sufficient following/agreement,
> >> after all.  :-)
> > 
> > If the goal is to support -Wshadow then it would be nice to settle on a style
> > so it is consistent across the tree.
> 
> It's not really about -Wshadow.  In C++, in order to use a
> member initializer, like in: 
> 
>   cmdarg (cmdarg_kind type_, char *string_)
>     : type (type_), string (string_)
>   {}
> 
> The parameter names really must be different from the
> struct's elements.
> 
> This:
> 
>   cmdarg (cmdarg_kind type, char *string)
>     : this->type (type), this->string (string)
>   {}
> 
> is not valid C++ and does _not_ compile:

No, but this compiles:

   cmdarg (cmdarg_kind type, char *string)
     : type (type), string (string)
   {}

I converted a non-trivial C++ code base both to C++11 and to compile with
-Wshadow (two of several different passes).  There were many instances of the
style above that all had to be changed to compile with -Wshadow.

-- 
John Baldwin

Re: [PATCH] gdb: Use vector::emplace_back

[Pedro Alves]

On 11/09/2016 05:08 PM, John Baldwin wrote:

>> This:
>>
>>   cmdarg (cmdarg_kind type, char *string)
>>     : this->type (type), this->string (string)
>>   {}
>>
>> is not valid C++ and does _not_ compile:
> 
> No, but this compiles:
> 
>    cmdarg (cmdarg_kind type, char *string)
>      : type (type), string (string)
>    {}
> 
> I converted a non-trivial C++ code base both to C++11 and to compile with
> -Wshadow (two of several different passes).  There were many instances of the
> style above that all had to be changed to compile with -Wshadow.

Ah!  I stand corrected.  Then it is about -Wshadow after all.

Thanks,
Pedro Alves