Drop a distracting function from locate(1)

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

Drop a distracting function from locate(1)

Michael McConville-2
Index: locate/fastfind.c
===================================================================
RCS file: /cvs/src/usr.bin/locate/locate/fastfind.c,v
retrieving revision 1.12
diff -u -p -r1.12 fastfind.c
--- locate/fastfind.c 16 Jan 2015 06:40:09 -0000 1.12
+++ locate/fastfind.c 19 Sep 2015 18:28:30 -0000
@@ -65,7 +65,8 @@ statistic (fp, path_fcodes)
  } else
  count += c - OFFSET;
 
- sane_count(count);
+ if (count < 0 || count >= PATH_MAX)
+ errx(1, "corrupted database");
  for (p = path + count; (c = getc(fp)) > SWITCH; size++)
  if (c < PARITY) {
  if (c == UMLAUT) {
@@ -212,7 +213,8 @@ fastfind
  count += c - OFFSET;
  }
 
- sane_count(count);
+ if (count < 0 || count >= PATH_MAX)
+ errx(1, "corrupted database");
  /* overlay old path */
  p = path + count;
  foundchar = p - 1;
Index: locate/locate.c
===================================================================
RCS file: /cvs/src/usr.bin/locate/locate/locate.c,v
retrieving revision 1.26
diff -u -p -r1.26 locate.c
--- locate/locate.c 16 Jan 2015 06:40:09 -0000 1.26
+++ locate/locate.c 19 Sep 2015 18:28:30 -0000
@@ -331,15 +331,6 @@ usage(void)
  exit(1);
 }
 
-void
-sane_count(int count)
-{
- if (count < 0 || count >= PATH_MAX) {
- fprintf(stderr, "locate: corrupted database\n");
- exit(1);
- }
-}
-
 /* load fastfind functions */
 
 /* statistic */

Reply | Threaded
Open this post in threaded view
|

Re: Drop a distracting function from locate(1)

Benny Lofgren
On 2015-09-19 20:29, Michael McConville wrote:

> Index: locate/fastfind.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/locate/locate/fastfind.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 fastfind.c
> --- locate/fastfind.c 16 Jan 2015 06:40:09 -0000 1.12
> +++ locate/fastfind.c 19 Sep 2015 18:28:30 -0000
> @@ -65,7 +65,8 @@ statistic (fp, path_fcodes)
>   } else
>   count += c - OFFSET;
>  
> - sane_count(count);
> + if (count < 0 || count >= PATH_MAX)
> + errx(1, "corrupted database");
>   for (p = path + count; (c = getc(fp)) > SWITCH; size++)
>   if (c < PARITY) {
>   if (c == UMLAUT) {
> @@ -212,7 +213,8 @@ fastfind
>   count += c - OFFSET;
>   }
>  
> - sane_count(count);
> + if (count < 0 || count >= PATH_MAX)
> + errx(1, "corrupted database");
>   /* overlay old path */
>   p = path + count;
>   foundchar = p - 1;
> Index: locate/locate.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/locate/locate/locate.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 locate.c
> --- locate/locate.c 16 Jan 2015 06:40:09 -0000 1.26
> +++ locate/locate.c 19 Sep 2015 18:28:30 -0000
> @@ -331,15 +331,6 @@ usage(void)
>   exit(1);
>  }
>  
> -void
> -sane_count(int count)
> -{
> - if (count < 0 || count >= PATH_MAX) {
> - fprintf(stderr, "locate: corrupted database\n");
> - exit(1);
> - }
> -}
> -
>  /* load fastfind functions */
>  
>  /* statistic */

What's your thinking behind this? To me this seems like a perfectly
rational and well motivated function to have, both for readability and
rather than having to repeat the same statements several times over in
the rest of the code, risking future calamity if a change is made in one
place and not the other...

I mean, I don't know if I'm unusual, but I often find myself breaking
out separate functions from larger pieces of code, that may end up being
called just that one time in a program, just to improve readability and
keep things logically apart.


Regards,
/Benny

Reply | Threaded
Open this post in threaded view
|

Re: Drop a distracting function from locate(1)

Michael McConville-2
> What's your thinking behind this? To me this seems like a perfectly
> rational and well motivated function to have, both for readability and
> rather than having to repeat the same statements several times over in
> the rest of the code, risking future calamity if a change is made in
> one place and not the other...
>
> I mean, I don't know if I'm unusual, but I often find myself breaking
> out separate functions from larger pieces of code, that may end up
> being called just that one time in a program, just to improve
> readability and keep things logically apart.

The most important thing to note is that this diff removes seven lines.
If you're abstracting something into a function, it definitely shouldn't
be creating more code.

When you create functions for things like if (z) errx(...); conditions,
it makes the code much harder to read. Someone trying to figure out
what's going on has to find the function definition and remember what it
does. Extrapolate this for every two-line error condition, and you have
a major headache. Such functions improve readability only if you're the
one writing them.

So, I wouldn't like this function no matter how many times it was used.
And it only appears twice.

Reply | Threaded
Open this post in threaded view
|

Re: Drop a distracting function from locate(1)

Tobias Stoeckmann-5
On Sat, Sep 19, 2015 at 05:57:23PM -0400, Michael McConville wrote:
> If you're abstracting something into a function, it definitely shouldn't
> be creating more code.

Yet this shouldn't stop you from performing "divide and conquer". It's
not just about reducing lines of code when abstracting logical blocks,
i.e. functions.

If you want an example of what I mean, please look at the main function
of src/sbin/newfs_msdos/newfs_msdos.c.

> When you create functions for things like if (z) errx(...); conditions,
> it makes the code much harder to read.

Is that statement true for xmalloc(), too? ;)

On Sat, Sep 19, 2015 at 02:29:56PM -0400, Michael McConville wrote:
> - sane_count(count);
> + if (count < 0 || count >= PATH_MAX)
> + errx(1, "corrupted database");
>   for (p = path + count; (c = getc(fp)) > SWITCH; size++)

> - sane_count(count);
> + if (count < 0 || count >= PATH_MAX)
> + errx(1, "corrupted database");
>   /* overlay old path */
>   p = path + count;

I would recommend to replace PATH_MAX with sizeof (path) here, so it
looks less magical and adapts whenever someone changes path's size. I've
kept the very next line after your patches in my quote, which shows that
count is used as an index into path, therefore the sizeof (path) range
check.


Tobias

Reply | Threaded
Open this post in threaded view
|

Re: Drop a distracting function from locate(1)

Benny Lofgren
In reply to this post by Michael McConville-2
On 2015-09-19 23:57, Michael McConville wrote:

>> What's your thinking behind this? To me this seems like a perfectly
>> rational and well motivated function to have, both for readability and
>> rather than having to repeat the same statements several times over in
>> the rest of the code, risking future calamity if a change is made in
>> one place and not the other...
>>
>> I mean, I don't know if I'm unusual, but I often find myself breaking
>> out separate functions from larger pieces of code, that may end up
>> being called just that one time in a program, just to improve
>> readability and keep things logically apart.
>
> The most important thing to note is that this diff removes seven lines.
> If you're abstracting something into a function, it definitely shouldn't
> be creating more code.
>
> When you create functions for things like if (z) errx(...); conditions,
> it makes the code much harder to read. Someone trying to figure out
> what's going on has to find the function definition and remember what it
> does. Extrapolate this for every two-line error condition, and you have
> a major headache. Such functions improve readability only if you're the
> one writing them.
>
> So, I wouldn't like this function no matter how many times it was used.
> And it only appears twice.

I respectfully disagree. :-)  Not only do I feel this function actually
makes the code *easier* to read, even though one has to fish around for
the function elsewhere first, it also makes maintenance more coherent
and safer, in part due to what I mentioned above about for example
accidentally changing say all but one occurances of the unrolled code...

I also don't think one needs to be afraid to create more code in the
name of clarity, at least not as in this case when it is a matter of
only a few bytes that will likely be swallowed up by page size alignment
anyway. And although the savings in execution time from inlining the
function is always a factor to take into account I have a feeling it is
negligible in this case.

Further, if code readability is an argument, something that would have a
much, much bigger impact than this diff would be to take a look at and
try to do something about the horrible #ifdef madness in this piece of
code. I mean, just look at some of the function declarations! They're
making my eyes hurt...


Finally, of course I am aware I don't really have a say in the matter.
:-) I am just a little afraid of diffs that seem more for the sake of
change than for any particular benefit, but as long as there is a sound
thought behind I'll gladly shut up!

And in any case, this is just the opinion of someone who has read and
written unix code since before the Napoleonic wars (and is the proud
owner of not one but two first edition K&R:s, printed in 1978), so my
opinions on style are likely as old and antiquated as I am (or at least
feel some mornings).


Regards,

/Benny

Reply | Threaded
Open this post in threaded view
|

Re: Drop a distracting function from locate(1)

Otto Moerbeek
In reply to this post by Michael McConville-2
On Sat, Sep 19, 2015 at 05:57:23PM -0400, Michael McConville wrote:

> > What's your thinking behind this? To me this seems like a perfectly
> > rational and well motivated function to have, both for readability and
> > rather than having to repeat the same statements several times over in
> > the rest of the code, risking future calamity if a change is made in
> > one place and not the other...
> >
> > I mean, I don't know if I'm unusual, but I often find myself breaking
> > out separate functions from larger pieces of code, that may end up
> > being called just that one time in a program, just to improve
> > readability and keep things logically apart.
>
> The most important thing to note is that this diff removes seven lines.
> If you're abstracting something into a function, it definitely shouldn't
> be creating more code.
>
> When you create functions for things like if (z) errx(...); conditions,
> it makes the code much harder to read. Someone trying to figure out
> what's going on has to find the function definition and remember what it
> does. Extrapolate this for every two-line error condition, and you have
> a major headache. Such functions improve readability only if you're the
> one writing them.
>
> So, I wouldn't like this function no matter how many times it was used.
> And it only appears twice.

I do not agree. You only have to remeber "that function does sensible
error checking" and you do not have to remember at each spot which
condition is the right one.

Function are the major way of structuring code, use them.

        -Otto

Reply | Threaded
Open this post in threaded view
|

Re: Drop a distracting function from locate(1)

Michael McConville-2
Otto Moerbeek wrote:
> I do not agree. You only have to remeber "that function does sensible
> error checking" and you do not have to remember at each spot which
> condition is the right one.
>
> Function are the major way of structuring code, use them.

It's not the idea of functions that I'm questioning. I don't think
making a function for a simple if (x) then errx(...); condition is sane,
though. I'm a bit biased at the moment because I'm searching through the
tree for "creative" replacements for errx() et al.

Reply | Threaded
Open this post in threaded view
|

Re: Drop a distracting function from locate(1)

Otto Moerbeek
On Sun, Sep 20, 2015 at 07:43:46AM -0400, Michael McConville wrote:

> Otto Moerbeek wrote:
> > I do not agree. You only have to remeber "that function does sensible
> > error checking" and you do not have to remember at each spot which
> > condition is the right one.
> >
> > Function are the major way of structuring code, use them.
>
> It's not the idea of functions that I'm questioning. I don't think
> making a function for a simple if (x) then errx(...); condition is sane,
> though. I'm a bit biased at the moment because I'm searching through the
> tree for "creative" replacements for errx() et al.

If x is an expression involving relational operators, I do not like to
see multiple copies of that. e.g. verifying if a condition should use
> or >= is often hard enough. If I have to check that in multiple
places, I'm wasting effort. Also, if I see a bug and fix it, I do like
the fact that I do not have to check multiple occurrences of the same
expression and decide if they are really the same or not.

So even if a function increases the number of lines in a program,
often it reduces complexity.

        -Otto

Reply | Threaded
Open this post in threaded view
|

Re: Drop a distracting function from locate(1)

Ted Unangst-6
In reply to this post by Michael McConville-2
Michael McConville wrote:

I'm late to the party, but we could tidy things up a bit by moving the
function into the c file it's used in and using errx() internally.

> Index: locate/fastfind.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/locate/locate/fastfind.c,v
> retrieving revision 1.12
> diff -u -p -r1.12 fastfind.c
> --- locate/fastfind.c 16 Jan 2015 06:40:09 -0000 1.12
> +++ locate/fastfind.c 19 Sep 2015 18:28:30 -0000
> @@ -65,7 +65,8 @@ statistic (fp, path_fcodes)
>   } else
>   count += c - OFFSET;
>  
> - sane_count(count);
> + if (count < 0 || count >= PATH_MAX)
> + errx(1, "corrupted database");
>   for (p = path + count; (c = getc(fp)) > SWITCH; size++)
>   if (c < PARITY) {
>   if (c == UMLAUT) {
> @@ -212,7 +213,8 @@ fastfind
>   count += c - OFFSET;
>   }
>  
> - sane_count(count);
> + if (count < 0 || count >= PATH_MAX)
> + errx(1, "corrupted database");
>   /* overlay old path */
>   p = path + count;
>   foundchar = p - 1;
> Index: locate/locate.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/locate/locate/locate.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 locate.c
> --- locate/locate.c 16 Jan 2015 06:40:09 -0000 1.26
> +++ locate/locate.c 19 Sep 2015 18:28:30 -0000
> @@ -331,15 +331,6 @@ usage(void)
>   exit(1);
>  }
>  
> -void
> -sane_count(int count)
> -{
> - if (count < 0 || count >= PATH_MAX) {
> - fprintf(stderr, "locate: corrupted database\n");
> - exit(1);
> - }
> -}
> -
>  /* load fastfind functions */
>  
>  /* statistic */
>

Reply | Threaded
Open this post in threaded view
|

Re: Drop a distracting function from locate(1)

lists-2
In reply to this post by Otto Moerbeek
On Mon, 21 Sep 2015 07:47:55 +0200
Otto Moerbeek <[hidden email]> wrote:

> On Sun, Sep 20, 2015 at 07:43:46AM -0400, Michael McConville wrote:
>
> > Otto Moerbeek wrote:
> > > I do not agree. You only have to remeber "that function does sensible
> > > error checking" and you do not have to remember at each spot which
> > > condition is the right one.
> > >
> > > Function are the major way of structuring code, use them.
> >
> > It's not the idea of functions that I'm questioning. I don't think
> > making a function for a simple if (x) then errx(...); condition is sane,
> > though. I'm a bit biased at the moment because I'm searching through the
> > tree for "creative" replacements for errx() et al.
>
> If x is an expression involving relational operators, I do not like to
> see multiple copies of that. e.g. verifying if a condition should use
> > or >= is often hard enough. If I have to check that in multiple
> places, I'm wasting effort. Also, if I see a bug and fix it, I do like
> the fact that I do not have to check multiple occurrences of the same
> expression and decide if they are really the same or not.
>
> So even if a function increases the number of lines in a program,
> often it reduces complexity.

We're all thinking about resulting machine cycles added on top of
visual code appeal, clarity and simplicity, right? There is also a
road map, while striking the right balance, one day "just another
OpenBSD user" will want the code to be of learning quality for his
kids. Locate is working OK as long as no new bugs creep in and
efficiency is kept, fuzz it.

Reply | Threaded
Open this post in threaded view
|

Re: Drop a distracting function from locate(1)

Bob Beck-2


Holy bikeshed batman.. Did I accidentally stumble into a FreeBSD list?