Thread: Small miscellaneus fixes (Part II)

On Fri, Nov 25, 2022 at 06:27:04PM -0300, Ranier Vilela wrote:
> 5. Use boolean operator with boolean operands
> (b/src/backend/commands/tablecmds.c)

tablecmds.c: right.  Since 074c5cfbf

pg_dump.c: right.  Since b08dee24a

> 4. Fix dead code (src/backend/utils/adt/formatting.c)
> Np->sign == '+', is different than "!= '-'" and is different than "!= '+'"
> So the else is never hit.

formatting.c: I don't see the problem.

    if (Np->sign != '-')
    ...
    else if (Np->sign != '+' && IS_PLUS(Np->Num))
    ...

You said that the "else" is never hit, but why ?
What if sign is 0 ?

-- 
Justin

On Tue, Dec 20, 2022 at 06:51:34PM -0600, Justin Pryzby wrote:
> On Fri, Nov 25, 2022 at 06:27:04PM -0300, Ranier Vilela wrote:
> > 5. Use boolean operator with boolean operands
> > (b/src/backend/commands/tablecmds.c)
>
> tablecmds.c: right.  Since 074c5cfbf

Most of this does not seem to be really worth poking at.

    newcons = AddRelationNewConstraints(rel, NIL,
                                        list_make1(copyObject(constr)),
-                                       recursing | is_readd,   /* allow_merge */
+                                       recursing || is_readd,  /* allow_merge */
There is no damage here, but that looks like a typo so no objections
on this one.
--
Michael

On Thu, Dec 22, 2022 at 02:29:11PM -0300, Ranier Vilela wrote:
> Em ter., 20 de dez. de 2022 às 21:51, Justin Pryzby <pryzby@telsasoft.com> escreveu:
> 
> > On Fri, Nov 25, 2022 at 06:27:04PM -0300, Ranier Vilela wrote:
> > > 5. Use boolean operator with boolean operands
> > > (b/src/backend/commands/tablecmds.c)
> >
> > tablecmds.c: right.  Since 074c5cfbf
> >
> > pg_dump.c: right.  Since b08dee24a
> >
> > > 4. Fix dead code (src/backend/utils/adt/formatting.c)
> > > Np->sign == '+', is different than "!= '-'" and is different than "!=
> > '+'"
> > > So the else is never hit.
> >
> > formatting.c: I don't see the problem.
> >
> >         if (Np->sign != '-')
> >         ...
> >         else if (Np->sign != '+' && IS_PLUS(Np->Num))
> >         ...
> >
> > You said that the "else" is never hit, but why ?
>
> This is a Coverity report.
> 
> dead_error_condition: The condition Np->Num->flag & 0x200 cannot be true.
> 5671                        else if (Np->sign != '+' && IS_PLUS(Np->Num))
> 
> CID 1501076 (#1 of 1): Logically dead code (DEADCODE)dead_error_line: Execution
> cannot reach this statement: Np->Num->flag &= 0xffffffff....
> 
> So, the dead code is because IS_PUS(Np->Num) is already tested and cannot
> be true on else.

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.

-- 
Justin

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