httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dean Gaudet <dgau...@arctic.org>
Subject Re: cvs commit: apache-1.3/src/modules/standard mod_autoindex.c
Date Tue, 07 Jul 1998 17:58:20 GMT


On Tue, 7 Jul 1998, Rodent of Unusual Size wrote:

> dgaudet@hyperreal.org wrote:
> > 
> > dgaudet     98/07/06 21:54:05
> > 
> >   Modified:    src/modules/standard mod_autoindex.c
> >   Log:
> >   - remove unnecessary complexity and verbiage (backport dsortf from apache-nspr)
> 	:
> >   -    if (!result) {
> >   -     result = strcmp((*e1)->name, (*e2)->name);
> >   +        result = strcmp(c1->desc ? c1->desc : "", c2->desc ? c2->desc
: "");
> >   +        if (result) {
> >   +            return result;
> >   +        }
> >   +        break;
> 
> You know, I don't object to improving performance, but I do rather
> object to easy-to-read commented code being replaced with uncommented
> harder-to-read stuff.

That change was not so much a performance improvement as it was to make
a 120 line function into a 60 line function by eliminating unnecessary
state variables, and dozens of three line comments explaining the obvious.
Sure it also eliminated code in the common path (name comparisons) --
the name can't be NULL (a quick inspection of the rest of the module
will prove that).

To be honest Ken, it's not necessary to comment every line of code.
I find folks learn that when they're in school -- it's something that
the schools get completely wrong.  Grading proportional to the number
of comments is a crime that leads to folks who comment every line of code.

    i = i + 1;  /* add one to i */
    if (i != 0) {
	/* i isn't zero */
	...
    }

is not so different from:

    /*
     * Two valid strings, compare normally.
     */
    if ((s1 != NULL) && (s2 != NULL)) {
	result = strcmp(s1, s2);
    }
    /*
     * Two NULL strings - primary keys are equal (fake it).
     */
    else if ((s1 == NULL) && (s2 == NULL)) {
	result = 0;
    }
    /*
     * s1 is NULL, but s2 is a string - so s2 wins.
     */
    else if (s1 == NULL) {
	result = -1;
    }
    /*
     * Last case: s1 is a string and s2 is NULL, so s1 wins.
     */
    else {
	result = 1;
    }

to use an example that I deleted from your code, and replaced with the
small code above which you take issue with.

> The main 'unnecessary complexity' you appear to have removed is for
> the compiler, not for humans.  Fine, sometimes - except that the
> compilers don't maintain the code.  If you're going to do that, and
> make it harder to read, please add comments.

I'm sorry if you find it harder to read.  But it's so obvious that I
really don't think it needs comments. 

Dean


Mime
View raw message