Re: Small miscellaneus fixes (Part II) - Mailing list pgsql-hackers

On Thu, Jan 12, 2023 at 12:34 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> On Thu, Jan 12, 2023 at 12:15:24PM +0700, John Naylor wrote:
> > On Fri, Dec 23, 2022 at 8:08 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> >
> > > Makes sense now (in your first message, you said that the problem was
> > > with "sign", and the patch didn't address the actual problem in
> > > IS_PLUS()).
> > >
> > > One can look and find that the unreachable code was introduced at
> > > 7a3e7b64a.
> > >
> > > With your proposed change, the unreachable line is hit by regression
> > > tests, which is an improvment.  As is the change to pg_dump.c.
> >
> > But that now reachable line just unsets a flag that we previously found
> > unset, right?
>
> Good point.
>
> > And if that line was unreachable, then surely the previous flag-clearing
> > operation is too?
> >
> > 5669      994426 :                 if (IS_MINUS(Np->Num)) // <- also always
> > false
> > 5670           0 :                     Np->Num->flag &= ~NUM_F_MINUS;
> > 5671             :             }
> > 5672         524 :             else if (Np->sign != '+' && IS_PLUS(Np->Num))
> > 5673           0 :                 Np->Num->flag &= ~NUM_F_PLUS;
> >
> > https://coverage.postgresql.org/src/backend/utils/adt/formatting.c.gcov.html
> >
> > I'm inclined to turn the dead unsets into asserts.
>
> To be clear - did you mean like this ?
>
> diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
> index a4b524ea3ac..848956879f5 100644
> --- a/src/backend/utils/adt/formatting.c
> +++ b/src/backend/utils/adt/formatting.c
> @@ -5662,15 +5662,13 @@ NUM_processor(FormatNode *node, NUMDesc *Num, char *inout,
>                 }
>                 else
>                 {
> +                       Assert(!IS_MINUS(Np->Num));
> +                       Assert(!IS_PLUS(Np->Num));
>                         if (Np->sign != '-')
>                         {
>                                 if (IS_BRACKET(Np->Num) && IS_FILLMODE(Np->Num))
>                                         Np->Num->flag &= ~NUM_F_BRACKET;
> -                               if (IS_MINUS(Np->Num))
> -                                       Np->Num->flag &= ~NUM_F_MINUS;
>                         }
> -                       else if (Np->sign != '+' && IS_PLUS(Np->Num))
> -                               Np->Num->flag &= ~NUM_F_PLUS;
>
>                         if (Np->sign == '+' && IS_FILLMODE(Np->Num) && IS_LSIGN(Np->Num) == false)
>                                 Np->sign_wrote = true;  /* needn't sign */

I was originally thinking of something more localized: