Subject: Re: Small miscellaneus fixes (Part II)

From: Justin Pryzby <[email-masked]>
Date: Wed, 11 Jan 2023 23:34:07 -0600

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

---

From: John Naylor <[email-masked]>
Date: Thu, 12 Jan 2023 13:44:00 +0700

>                                 Np->sign_wrote = true;  /* needn't sign */

I was originally thinking of something more localized:

@@ -5666,11 +5666,10 @@ NUM_processor(FormatNode *node, NUMDesc *Num, char
*inout,
+            else if (Np->sign != '+')

...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

---

From: John Naylor <[email-masked]>
Date: Mon, 16 Jan 2023 13:28:02 +0700

I wrote:
> ...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.

The attached is what I mean. I'll commit this this week unless there are
objections.

--
John Naylor
EDB: http://www.enterprisedb.com
From 5dd409e6059be464ad635ce8cd885fc3de5d8e43 Mon Sep 17 00:00:00 2001
From: John Naylor <[email-masked]>
Date: Mon, 16 Jan 2023 13:03:22 +0700
Subject: [PATCH v4] Remove dead code in formatting.c

Remove some code guarded by IS_MINUS() or IS_PLUS(), where the entire
stanza is inside an else-block where both of these are false. This
should slightly improve test coverage.

While at it, remove coding that apparently assumes that unsetting a
bit is so expensive that we have to first check if it's already set
in the first place.

Reviewed by Justin Pryzby
Per Coverity report from Ranier Vilela

Discussion: https://www.postgresql.org/message-id/20221223010818.GP1153%40telsasoft.com
---
 src/backend/utils/adt/formatting.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/src/backend/utils/adt/formatting.c b/src/backend/utils/adt/formatting.c
index a4b524ea3a..f3f4db5ef6 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -5664,13 +5664,9 @@ NUM_processor(FormatNode *node, NUMDesc *Num, char *inout,
 		{
 			if (Np->sign != '-')
 			{
-				if (IS_BRACKET(Np->Num) && IS_FILLMODE(Np->Num))
+				if (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;
 
2.39.0