Thread

1. [PATCH] Cleanup: Compare pointers to NULL instead of 0

   Marti Raudsepp <marti@juffo.org> — 2010-10-29T16:25:03Z

   Coccinelle found a few places in the code where pointer expressions
   were compared to 0. I have changed them to NULL instead.
   
   There was one line that I didn't dare to touch, which looks like a
   false positive.
   
   src/backend/regex/regc_lex.c:849:
   if (v->now - save == 0 || ((int) c > 0 && (int) c <= v->nsubexp))
   
   I couldn't find the definition of v (struct vars) anywhere. Is it
   comparing two pointers here? Should it be "v->now == save" instead?
   
   But this code doesn't originate from PostgreSQL, so maybe it's not
   worth making cleanups here.
   
   Regards,
   Marti
   

2. Re: [PATCH] Cleanup: Compare pointers to NULL instead of 0

   Tom Lane <tgl@sss.pgh.pa.us> — 2010-10-29T19:59:08Z

   Marti Raudsepp <marti@juffo.org> writes:
   > Coccinelle found a few places in the code where pointer expressions
   > were compared to 0. I have changed them to NULL instead.
   
   Applied, thanks!
   
   > There was one line that I didn't dare to touch, which looks like a
   > false positive.
   > src/backend/regex/regc_lex.c:849:
   > if (v->now - save == 0 || ((int) c > 0 && (int) c <= v->nsubexp))
   
   Hmm.  I think this is a false positive in the tool: the difference
   between two pointers is an integer, not a pointer, so comparison
   to an integer is the semantically correct thing to do --- in fact,
   I'd argue that s/0/NULL/ ought to lead to a warning here.
   
   However, by the same token, the code overspecifies the computation.
   We don't really need to compute the pointer difference, just compare
   the pointers for equality.  If it were all integer arithmetic I'd
   expect the compiler to figure that out, but given the type issues
   maybe not.  So I changed it to v->now == save as you suggest.  The
   only advantage of the previous coding would be if we wanted to change
   the test to check for some other number of characters consumed, which
   seems mighty unlikely.
   
   > I couldn't find the definition of v (struct vars) anywhere.
   
   It's near the head of regcomp.c.
   
   > But this code doesn't originate from PostgreSQL, so maybe it's not
   > worth making cleanups here.
   
   Well, neither is zic, but we still have to look at the warnings.
   
   			regards, tom lane
   
   

3. Re: [PATCH] Cleanup: Compare pointers to NULL instead of 0

   Alvaro Herrera <alvherre@commandprompt.com> — 2010-10-29T20:11:18Z

   Excerpts from Marti Raudsepp's message of vie oct 29 13:25:03 -0300 2010:
   > Coccinelle found a few places in the code where pointer expressions
   > were compared to 0. I have changed them to NULL instead.
   > 
   > There was one line that I didn't dare to touch, which looks like a
   > false positive.
   > 
   > src/backend/regex/regc_lex.c:849:
   > if (v->now - save == 0 || ((int) c > 0 && (int) c <= v->nsubexp))
   > 
   > I couldn't find the definition of v (struct vars) anywhere. Is it
   > comparing two pointers here? Should it be "v->now == save" instead?
   
   regcomp.c line 198.  Yes, it's a pointer.
   
   -- 
   Álvaro Herrera <alvherre@commandprompt.com>
   The PostgreSQL Company - Command Prompt, Inc.
   PostgreSQL Replication, Consulting, Custom Development, 24x7 support