Re: [PATCH] Make dwarf_expr_piece::pieces an std::vector

[Simon Marchi <simon dot marchi at polymtl dot ca>]

On 2017-09-14 12:24, Pedro Alves wrote:
> On 09/14/2017 10:26 AM, Simon Marchi wrote:
> > On 2017-09-14 11:21, Pedro Alves wrote:
>
> > > I wonder if there's ever a case for 'const T &&' parameters,
> > > and if GCC could warn about them.  At least in non-template
> > > functions.
>
> FYI, I filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82213 now.

Thanks, I couldn't have stated it as clearly as you did.

> > Oops, I started with a const reference, before changing it to rvalue
> > reference (and forgot to remove the const).  Does the patch look good 
> > to
> > you otherwise?
>
> Sure, with tiny nit below.
>
> >  /* Expand the memory allocated stack to contain at least
> > @@ -285,14 +282,9 @@ dwarf_expr_context::stack_empty_p () const
> >  void
> >  dwarf_expr_context::add_piece (ULONGEST size, ULONGEST offset)
> >  {
> > -  struct dwarf_expr_piece *p;
> > +  this->pieces.emplace_back ();
> > +  dwarf_expr_piece *p = &(this->pieces.back ());
>
> The extra parens look odd to me:
>
> - dwarf_expr_piece *p = &(this->pieces.back ());
> + dwarf_expr_piece *p = &this->pieces.back ();
>
> ('p' could be a reference instead, since it's always
> non-null, but I guess even better would be to move most
> of the code below to a dwarf_expr_piece ctor.  But you don't
> really have to do that.  I definitely understand not wanting
> to change too much at a time.  And I wouldn't have said a thing
> if it weren't for the parens making me pause.  :-))

I initially left it as a pointer out of laziness, but I've changed it to 
a reference now.  I am pushing the patch with this and the const rvalue 
reference fixed.

I tried to add some constructors, but I didn't know how to design it 
nicely to support different types of value locations.  Maybe the best 
would be to add some factory functions that build various kinds of 
pieces, and make the constructor private.  Something like:

struct dwarf_expr_piece
{
  static dwarf_expr_piece make_memory_piece (CORE_ADDR addr, bool 
in_stack_memory)
  static dwarf_expr_piece make_register_piece (int regno);

private:
  dwarf_expr_piece ();
};

I'll try that for a follow-up patch.

Simon