Date: Wed, 11 Jan 2023 23:34:07 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: John Naylor <john.naylor@enterprisedb.com>
Cc: Ranier Vilela <ranier.vf@gmail.com>, pgsql-hackers@postgresql.org
Subject: Re: Small miscellaneus fixes (Part II)
Message-ID: <20230112053407.GT9837@telsasoft.com>
References: <CAEudQAp2R2fbbi0OHHhv_n4=Ch0t1VtjObR9YMqtGKHJ+faUFQ@mail.gmail.com>
 <20221221005134.GJ1153@telsasoft.com>
 <CAEudQAq_1BfRhq6H6DHR_KAxYcnFjXKuWVuL7cDaBPKPtEf+4g@mail.gmail.com>
 <20221223010818.GP1153@telsasoft.com>
 <CAFBsxsEhZCcN2_-sJZgopJ2khqp1wAtjS3Ziery35OxDtssNWg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAFBsxsEhZCcN2_-sJZgopJ2khqp1wAtjS3Ziery35OxDtssNWg@mail.gmail.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: <https://www.postgresql.org/message-id/20230112053407.GT9837%40telsasoft.com>
Precedence: bulk

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