Hi list,

Since my previous Coccinelle cleanup patches have gained positive
feedback I decided to try some more.

This time I wrote my own spatches.

patch 0001 turns (a - b == 0) into (a == b) and similarly with !=
patch 0002 applies the same to operators >, >=, <, <=

I'm well aware that there's a point where code cleanups defeat their
purpose and become a burden. So this will probably be my last one,
I'll go to doing productive things instead. :)

Regards,
Marti

Search Discussions

  • Robert Haas at Nov 1, 2010 at 3:04 am

    On Fri, Oct 29, 2010 at 7:33 PM, Marti Raudsepp wrote:
    Since my previous Coccinelle cleanup patches have gained positive
    feedback I decided to try some more.

    This time I wrote my own spatches.

    patch 0001 turns (a - b == 0) into (a == b) and similarly with !=
    patch 0002 applies the same to operators >, >=, <, <=

    I'm well aware that there's a point where code cleanups defeat their
    purpose and become a burden. So this will probably be my last one,
    I'll go to doing productive things instead. :)
    I like the 0002 patch better than the 0001 patch. For example, consider:

    - while (q - (uint8 *) keybuf - 8)
    + while (q - (uint8 *) keybuf != 8)

    That's pretty darn inscrutable either way, and the loop that follows
    does absolutely nothing to clear anything up. What's so special about
    8? Are we counting up to 8 or down to 8? Most of these examples
    involve an additive constant, or (in the case of zic.c) are parallel
    to surrounding code that does similar tests with an additive constant.
    I'm pretty much inclined to think that these examples should either
    be left alone or given a more substantial face lift. On the other
    hand, most of the stuff in the 0002 patch takes this form:

    - if (p - tmpbuf > 0)
    + if (p > tmpbuf)

    IMHO, that's an improvement. However, I think that this rewrite
    changes the overflow behavior. There's a comment not too far from the
    bgwriter.c change that seems to imply that the test is written the way
    that it is specifically to produce a certain overflow behavior. The
    others all look OK, though, as far as I can tell.

    --
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
  • Tom Lane at Nov 1, 2010 at 3:56 pm

    Robert Haas writes:
    On Fri, Oct 29, 2010 at 7:33 PM, Marti Raudsepp wrote:
    patch 0001 turns (a - b == 0) into (a == b) and similarly with !=
    patch 0002 applies the same to operators >, >=, <, <=

    I'm well aware that there's a point where code cleanups defeat their
    purpose and become a burden. So this will probably be my last one,
    I'll go to doing productive things instead. :)
    I like the 0002 patch better than the 0001 patch.
    I'm not really thrilled with either of them, as I don't think they are
    doing much to improve readability. As for 0002, as you say, at least
    the change in bgwriter.c is actively breaking things. I'm not eager to
    go through and see which of the other changes there might be affecting
    overflow or signed-vs-unsigned comparison behavior.

    regards, tom lane

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
grouppgsql-hackers @
categoriespostgresql
postedOct 29, '10 at 11:33p
activeNov 1, '10 at 3:56p
posts3
users3
websitepostgresql.org...
irc#postgresql

People

Translate

site design / logo © 2021 Grokbase