Re: potential patch for gdb bug c++/20020

[Bob Steagall <bob.steagall.cpp@gmail.com>]

On Thu, Dec 6, 2018 at 2:20 PM Andrew Burgess
<andrew.burgess@embecosm.com> wrote:
>
> * Bob Steagall <bob.steagall.cpp@gmail.com> [2018-12-06 13:29:31 -0500]:
>
> > Description: This patch, against released version 8.2, fixes the
> > problem reported in gdb bug c++/20020, using the approach described in
> > comment 1 of that report.
> >
> > Changelog entry:
> >
> > 2018-12-06  Bob Steagall   <bob.steagall.cpp@gmail.com>
> >
> >         * cp-valprint.c: Fixes bug c++/20020.
>
> > --- gdb/cp-valprint.c 2018-09-05 03:27:13.000000000 -0400
> > +++ gdb/cp-valprint.c.new     2018-12-06 13:01:06.819266165 -0500
> > @@ -326,12 +326,16 @@ cp_print_value_fields (struct type *type
> >                     fprintf_filtered (stream,
> >                                       _("<error reading variable: %s>"),
> >                                       ex.message);
> > +                      v = NULL;
>
> I don't think this NULL assignment should be needed.  `v` starts as
> NULL, and we only end in this block if `value_static_field` throws an
> exception, which will be before `v` is assigned too.

Agreed.  I was being, perhaps, too paranoid here.

> >                   }
> >                 END_CATCH
> >
> > -               cp_print_static_field (TYPE_FIELD_TYPE (type, i),
> > -                                      v, stream, recurse + 1,
> > -                                      options);
> > +                  if (v != NULL)
> > +                    {
>
> You should drop the '{' and '}' here for a single statement block.

Disagree.  The gdb coding standard document specifically calls out
lines of code,
not statements:

    "Any two or more lines in code should be wrapped in braces, even if they
     are comments, as they look like separate statements"

> > +                      cp_print_static_field (TYPE_FIELD_TYPE (type, i),
> > +                                             v, stream, recurse + 1,
> > +                                             options);
> > +                    }
> >               }
> >             else if (i == vptr_fieldno && type == vptr_basetype)
> >               {
>
> I'm not a maintainer so can't approve patches, but this seems sensible
> to me.
>
> Thanks,
> Andrew

I will update and re-send.

Thanks for the review,
--Bob