You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by dg...@hyperreal.org on 1998/07/07 06:54:05 UTC
cvs commit: apache-1.3/src/modules/standard mod_autoindex.c
dgaudet 98/07/06 21:54:05
Modified: src/modules/standard mod_autoindex.c
Log:
- fix a gcc -Wall warning in dsortf() (Ken you should be using -Wall)
- remove unnecessary complexity and verbiage (backport dsortf from apache-nspr)
Revision Changes Path
1.85 +21 -74 apache-1.3/src/modules/standard/mod_autoindex.c
Index: mod_autoindex.c
===================================================================
RCS file: /export/home/cvs/apache-1.3/src/modules/standard/mod_autoindex.c,v
retrieving revision 1.84
retrieving revision 1.85
diff -u -r1.84 -r1.85
--- mod_autoindex.c 1998/06/20 11:20:39 1.84
+++ mod_autoindex.c 1998/07/07 04:54:04 1.85
@@ -1017,12 +1017,9 @@
static int dsortf(struct ent **e1, struct ent **e2)
{
- char *s1;
- char *s2;
struct ent *c1;
struct ent *c2;
int result = 0;
- int compare_by_string = 1;
/*
* First, see if either of the entries is for the parent directory.
@@ -1040,87 +1037,37 @@
*/
if ((*e1)->ascending) {
c1 = *e1;
- c2 = *e2;
+ c2 = *e2;
}
else {
c1 = *e2;
c2 = *e1;
}
- /*
- * Choose the right values for the sort keys.
- */
switch (c1->key) {
case K_LAST_MOD:
- /*
- * Since the last-modified time is a monotonically increasing integer,
- * we can short-circuit this process with a simple comparison.
- */
- result = c1->lm - c2->lm;
- if (result != 0) {
- result = (result > 0) ? 1 : -1;
- }
- compare_by_string = 0;
- break;
+ if (c1->lm > c2->lm) {
+ return 1;
+ }
+ else if (c1->lm < c2->lm) {
+ return -1;
+ }
+ break;
case K_SIZE:
- /*
- * We can pull the same trick with the size as with the mtime.
- */
- result = c1->size - c2->size;
- if (result != 0) {
- result = (result > 0) ? 1 : -1;
- }
- compare_by_string = 0;
- break;
+ if (c1->size > c2->size) {
+ return 1;
+ }
+ else if (c1->size < c2->size) {
+ return -1;
+ }
+ break;
case K_DESC:
- s1 = c1->desc;
- s2 = c2->desc;
- break;
- case K_NAME:
- default:
- s1 = c1->name;
- s2 = c2->name;
- break;
- }
-
- if (compare_by_string) {
- /*
- * Take some care, here, in case one string or the other (or both) is
- * NULL.
- */
-
- /*
- * 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;
- }
- }
- /*
- * If the keys were equal, the file name is *always* the secondary key -
- * in ascending order.
- */
- if (!result) {
- result = strcmp((*e1)->name, (*e2)->name);
+ result = strcmp(c1->desc ? c1->desc : "", c2->desc ? c2->desc : "");
+ if (result) {
+ return result;
+ }
+ break;
}
- return result;
+ return strcmp(c1->name, c2->name);
}
Re: cvs commit: apache-1.3/src/modules/standard mod_autoindex.c
Posted by Dean Gaudet <dg...@arctic.org>.
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
Re: cvs commit: apache-1.3/src/modules/standard mod_autoindex.c
Posted by Marc Slemko <ma...@worldgate.com>.
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.
Same here:
Index: suexec.c
===================================================================
RCS file: /export/home/cvs/apache-1.3/src/support/suexec.c,v
retrieving revision 1.39
retrieving revision 1.41
diff -u -r1.39 -r1.41
--- suexec.c 1998/06/18 19:06:56 1.39
+++ suexec.c 1998/07/01 10:34:20 1.41
@@ -258,7 +259,40 @@
*/
prog = argv[0];
if (argc < 4) {
- log_err("too few arguments\n");
+ char msgbuf[2048];
+ int i;
+ int clen;
+ static char *omsg = " {buffer overflow}";
+ int olen = strlen(omsg);
+
+ ap_snprintf(msgbuf, sizeof(msgbuf), "too few (%d) arguments:", argc);
+ clen = strlen(msgbuf);
+ for (i = 0; i < argc; i++) {
+ int alen = strlen(argv[i]) + 4;
+ int rlen = sizeof(msgbuf) - clen - 1;
+ int oflow = (alen > rlen);
+
+ alen = oflow ? rlen : alen;
+ if (rlen > 1) {
+ msgbuf[clen++] = ' ';
+ alen--;
+ }
+ if (rlen > 2) {
+ msgbuf[clen++] = '[';
+ alen--;
+ }
+ ap_cpystrn(&msgbuf[clen], argv[i], alen);
+ if (oflow) {
+ ap_cpystrn(&msgbuf[sizeof(msgbuf) - olen - 1], omsg, olen + 1);
+ break;
+ }
+ else {
+ clen += alen - 2;
+ msgbuf[clen++] = ']';
+ msgbuf[clen] = '\0';
+ }
+ }
+ log_err("%s\n", msgbuf);
exit(101);
}
target_uname = argv[1];
I sure as hell can't figure out if that is safe at a glance, so it sure
isn't code I want in my suexec...
Re: cvs commit: apache-1.3/src/modules/standard mod_autoindex.c
Posted by Rodent of Unusual Size <Ke...@Golux.Com>.
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.
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.
#ken P-)}
Ken Coar <http://Web.Golux.Com/coar/>
Apache Group member <http://www.apache.org/>
"Apache Server for Dummies" <http://Web.Golux.Com/coar/ASFD/>
Re: cvs commit: apache-1.3/src/modules/standard mod_autoindex.c
Posted by Dean Gaudet <dg...@arctic.org>.
Oh ok, sorry. There was a -Wall in fnmatch a week ago too which went
away... so I was guessing that you weren't using -Wall.
Dean
On Tue, 7 Jul 1998, Rodent of Unusual Size wrote:
> dgaudet@hyperreal.org wrote:
> >
> > - fix a gcc -Wall warning in dsortf() (Ken you should be using -Wall)
>
> I *am*. It's not reporting a thing.
>
> #ken P-)}
>
> Ken Coar <http://Web.Golux.Com/coar/>
> Apache Group member <http://www.apache.org/>
> "Apache Server for Dummies" <http://Web.Golux.Com/coar/ASFD/>
>
Re: cvs commit: apache-1.3/src/modules/standard mod_autoindex.c
Posted by Rodent of Unusual Size <Ke...@Golux.Com>.
dgaudet@hyperreal.org wrote:
>
> - fix a gcc -Wall warning in dsortf() (Ken you should be using -Wall)
I *am*. It's not reporting a thing.
#ken P-)}
Ken Coar <http://Web.Golux.Com/coar/>
Apache Group member <http://www.apache.org/>
"Apache Server for Dummies" <http://Web.Golux.Com/coar/ASFD/>