On Thu, Jan 12, 2023 at 12:15:24PM +0700, John Naylor wrote: > On Fri, Dec 23, 2022 at 8:08 AM Justin Pryzby <pry...@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 */
