Thread: unused code in float8_to_char , formatting.c ?
Hi Guys, Looking around the code Today, one of my helpful tools detected this dead code. As far as I can see, it is actually unused call to strlen() in formatting.c, float8_to_char(). Diff attached. -- GJ
Attachment
On Thu, Apr 4, 2013 at 6:47 PM, Greg Jaskiewicz <gryzman@me.com> wrote: > Looking around the code Today, one of my helpful tools detected this dead code. > As far as I can see, it is actually unused call to strlen() in formatting.c, float8_to_char(). I poked at this a little and suggest the following somewhat more extensive cleanup. It seems to me that there are a bunch of these functions where len is unconditionally initialized in NUM_TOCHAR_prepare and then used there. Similarly in NUM_TOCHAR_cleanup. And then there's a chunk of each individual function that does it a third time. Rather than use the same variable in all three places, I've moved the variable declarations to the innermost possible scope. Doing that revealed a bunch of other, similar places where we can get rid of strlen() calls. Does this version seem like a good idea? ...Robert
Attachment
On 7 Apr 2013, at 05:14, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Apr 4, 2013 at 6:47 PM, Greg Jaskiewicz <gryzman@me.com> wrote: >> Looking around the code Today, one of my helpful tools detected this dead code. >> As far as I can see, it is actually unused call to strlen() in formatting.c, float8_to_char(). > > I poked at this a little and suggest the following somewhat more > extensive cleanup. > > It seems to me that there are a bunch of these functions where len is > unconditionally initialized in NUM_TOCHAR_prepare and then used there. > Similarly in NUM_TOCHAR_cleanup. And then there's a chunk of each > individual function that does it a third time. Rather than use the > same variable in all three places, I've moved the variable > declarations to the innermost possible scope. Doing that revealed a > bunch of other, similar places where we can get rid of strlen() calls. > > Does this version seem like a good idea? Looks more extensive :-) On the quick glance, without a lot of testing it looks ok. But the lack of test cases stressing all different cases in that file, makes it impossible to say that there's no regressions. Personally I always feel uneasy making extensive changes in complicated code like this, without any integrated test case(s). -- GJ
On Sun, Apr 7, 2013 at 12:14:29AM -0400, Robert Haas wrote: > On Thu, Apr 4, 2013 at 6:47 PM, Greg Jaskiewicz <gryzman@me.com> wrote: > > Looking around the code Today, one of my helpful tools detected this dead code. > > As far as I can see, it is actually unused call to strlen() in formatting.c, float8_to_char(). > > I poked at this a little and suggest the following somewhat more > extensive cleanup. > > It seems to me that there are a bunch of these functions where len is > unconditionally initialized in NUM_TOCHAR_prepare and then used there. > Similarly in NUM_TOCHAR_cleanup. And then there's a chunk of each > individual function that does it a third time. Rather than use the > same variable in all three places, I've moved the variable > declarations to the innermost possible scope. Doing that revealed a > bunch of other, similar places where we can get rid of strlen() calls. > > Does this version seem like a good idea? Robert, were you going to apply this patch from April? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Sat, Nov 30, 2013 at 9:01 PM, Bruce Momjian <bruce@momjian.us> wrote: > On Sun, Apr 7, 2013 at 12:14:29AM -0400, Robert Haas wrote: >> On Thu, Apr 4, 2013 at 6:47 PM, Greg Jaskiewicz <gryzman@me.com> wrote: >> > Looking around the code Today, one of my helpful tools detected this dead code. >> > As far as I can see, it is actually unused call to strlen() in formatting.c, float8_to_char(). >> >> I poked at this a little and suggest the following somewhat more >> extensive cleanup. >> >> It seems to me that there are a bunch of these functions where len is >> unconditionally initialized in NUM_TOCHAR_prepare and then used there. >> Similarly in NUM_TOCHAR_cleanup. And then there's a chunk of each >> individual function that does it a third time. Rather than use the >> same variable in all three places, I've moved the variable >> declarations to the innermost possible scope. Doing that revealed a >> bunch of other, similar places where we can get rid of strlen() calls. >> >> Does this version seem like a good idea? > > Robert, were you going to apply this patch from April? Done. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company