Re: Small miscellaneus fixes (Part II)

On Thu, Jan 12, 2023 at 12:34 PM Justin Pryzby <pryzby(at)telsasoft(dot)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(at)telsasoft(dot)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:

--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -5666,11 +5666,10 @@ NUM_processor(FormatNode *node, NUMDesc *Num, char
*inout,
{
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;
+ Assert(!IS_MINUS(Np->Num));
}
- else if (Np->sign != '+' && IS_PLUS(Np->Num))
- Np->Num->flag &= ~NUM_F_PLUS;
+ else if (Np->sign != '+')
+ Assert(!IS_PLUS(Np->Num));

...but arguably the earlier check is close enough that it's silly to assert
in the "else" branch, and I'd be okay with just nuking those lines. Another
thing that caught my attention is the assumption that unsetting a bit is so
expensive that we have to first check if it's set, so we may as well remove
"IS_BRACKET(Np->Num)" as well.

--
John Naylor
EDB: http://www.enterprisedb.com