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 */
