public inbox for [email protected]
help / color / mirror / Atom feedFrom: Justin Pryzby <[email protected]>
To: John Naylor <[email protected]>
Cc: Ranier Vilela <[email protected]>
Cc: [email protected]
Subject: Re: Small miscellaneus fixes (Part II)
Date: Wed, 11 Jan 2023 23:34:07 -0600
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAFBsxsEhZCcN2_-sJZgopJ2khqp1wAtjS3Ziery35OxDtssNWg@mail.gmail.com>
References: <CAEudQAp2R2fbbi0OHHhv_n4=Ch0t1VtjObR9YMqtGKHJ+faUFQ@mail.gmail.com>
<[email protected]>
<CAEudQAq_1BfRhq6H6DHR_KAxYcnFjXKuWVuL7cDaBPKPtEf+4g@mail.gmail.com>
<[email protected]>
<CAFBsxsEhZCcN2_-sJZgopJ2khqp1wAtjS3Ziery35OxDtssNWg@mail.gmail.com>
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 protected]> 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 */
view thread (4+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Small miscellaneus fixes (Part II)
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox