[HACKERS] Patches for static check on geo_ops.c
Grzegorz Jaskiewicz wonderful static checker coughed up 5 errors in
geo_ops.c. None of them of any particular excitement or of earth
shattering nature. A patch is attached below that should correct these.
(The more little issue we eliminate, the more the large ones will stand
out.)
At line 3131 value stored into 'dist' variable is never referenced again.
At line 3014 value stored into 'dist' variable is never referenced again.
At line 2942 value stored into 'd' variable is never referenced again.
At line 2953 value stored into 'd' variable is never referenced again.
At line 2993 value stored into 'd' variable is never referenced again.
? patchfile_clang
Index: src/backend/utils/adt/geo_ops.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/utils/adt/geo_ops.c,v
retrieving revision 1.103
diff -c -r1.103 geo_ops.c
*** src/backend/utils/adt/geo_ops.c 28 Jul 2009 09:47:59 - 1.103
--- src/backend/utils/adt/geo_ops.c 27 Aug 2009 09:31:30 -
***
*** 2939,2945
memcpy(point, l1-p[1], sizeof(Point));
}
! if ((d = dist_ps_internal(l2-p[0], l1)) dist)
{
result = DatumGetPointP(DirectFunctionCall2(close_ps,
PointPGetDatum(l2-p[0]),
--- 2939,2945
memcpy(point, l1-p[1], sizeof(Point));
}
! if (dist_ps_internal(l2-p[0], l1) dist)
{
result = DatumGetPointP(DirectFunctionCall2(close_ps,
PointPGetDatum(l2-p[0]),
***
*** 2950,2956
LsegPGetDatum(l2)));
}
! if ((d = dist_ps_internal(l2-p[1], l1)) dist)
{
result = DatumGetPointP(DirectFunctionCall2(close_ps,
PointPGetDatum(l2-p[1]),
--- 2950,2956
LsegPGetDatum(l2)));
}
! if (dist_ps_internal(l2-p[1], l1) dist)
{
result = DatumGetPointP(DirectFunctionCall2(close_ps,
PointPGetDatum(l2-p[1]),
***
*** 2990,2996
point.x = box-low.x;
point.y = box-high.y;
statlseg_construct(lseg, box-low, point);
! dist = d = dist_ps_internal(pt, lseg);
statlseg_construct(seg, box-high, point);
if ((d = dist_ps_internal(pt, seg)) dist)
--- 2990,2996
point.x = box-low.x;
point.y = box-high.y;
statlseg_construct(lseg, box-low, point);
! dist = dist_ps_internal(pt, lseg);
statlseg_construct(seg, box-high, point);
if ((d = dist_ps_internal(pt, seg)) dist)
***
*** 3011,3017
statlseg_construct(seg, box-high, point);
if ((d = dist_ps_internal(pt, seg)) dist)
{
- dist = d;
memcpy(lseg, seg, sizeof(lseg));
}
--- 3011,3016
***
*** 3128,3134
statlseg_construct(seg, box-high, point);
if ((d = lseg_dt(lseg, seg)) dist)
{
- dist = d;
memcpy(bseg, seg, sizeof(bseg));
}
--- 3127,3132
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patches for static check on geo_ops.c
On 27 Aug 2009, at 10:46, Paul Matthews wrote: Grzegorz Jaskiewicz wonderful static checker coughed up 5 errors in geo_ops.c. None of them of any particular excitement or of earth shattering nature. A patch is attached below that should correct these. (The more little issue we eliminate, the more the large ones will stand out.) I am flattered, but I am merely a user of it - running it against postgresql's source. The checker is written by wonderful llvm developers, so you should thank them (and apple, for sponsoring development of the checker). -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patches for static check on geo_ops.c
Paul Matthews p...@netspace.net.au writes: Grzegorz Jaskiewicz wonderful static checker coughed up 5 errors in geo_ops.c. None of them of any particular excitement or of earth shattering nature. A patch is attached below that should correct these. (The more little issue we eliminate, the more the large ones will stand out.) At line 3131 value stored into 'dist' variable is never referenced again. At line 3014 value stored into 'dist' variable is never referenced again. At line 2942 value stored into 'd' variable is never referenced again. At line 2953 value stored into 'd' variable is never referenced again. At line 2993 value stored into 'd' variable is never referenced again. I've applied the first three of these changes, but not the last two (the 'dist' assignments). clang seems to have a tin ear for style :-(. It's failing to notice that we have several similar code blocks in sequence in these two places, and making the last one different from the rest would decrease code readability and modifiability. I'm happy if the compiler optimizes away useless stores, but I don't really think it should presume to dictate code style to us on that basis. BTW, if we did apply those changes, I suppose clang would immediately start complaining that the preceding assignments to 'd' are useless. So by the time we'd made it happy, those code blocks would look quite different from their mates. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patches for static check on geo_ops.c
Tom Lane wrote: I've applied the first three of these changes, but not the last two (the 'dist' assignments). clang seems to have a tin ear for style :-(. It's failing to notice that we have several similar code blocks in sequence in these two places, and making the last one different from the rest would decrease code readability and modifiability. voice=Maxwell SmartAh! The old programming via copy-and-paste trick/voice. Maybe clang's ear for style isn't that bad after all. :-) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
