You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jeff Trawick <tr...@gmail.com> on 2009/09/15 15:04:37 UTC
Re: svn commit: r814091 - in /httpd/httpd/trunk: CHANGES
support/htcacheclean.c
On Fri, Sep 11, 2009 at 7:57 PM, <mi...@apache.org> wrote:
> Author: minfrin
> Date: Fri Sep 11 23:57:48 2009
> New Revision: 814091
>
> URL: http://svn.apache.org/viewvc?rev=814091&view=rev
> Log:
> htcacheclean: 19 ways to fail, 1 error message. Fixed.
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/support/htcacheclean.c
>
>
> Modified: httpd/httpd/trunk/support/htcacheclean.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/support/htcacheclean.c?rev=814091&r1=814090&r2=814091&view=diff
>
> ==============================================================================
> --- httpd/httpd/trunk/support/htcacheclean.c (original)
> +++ httpd/httpd/trunk/support/htcacheclean.c Fri Sep 11 23:57:48 2009
> @@ -802,48 +806,48 @@
> break;
> }
> else if (status != APR_SUCCESS) {
> - usage();
> + usage(NULL);
> }
> else {
> switch (opt) {
> case 'i':
> if (intelligent) {
> - usage();
> + usage(apr_psprintf(pool, "The option '%c' cannot be
> specified more than once", (int)opt));
>
Why does opt need to be cast to (int)?
Why not simply zap all these checks of the form
if (silly user specified no-argument option again) {
remind them who is boss
}
to avoid code bloat?
Re: svn commit: r814091 - in /httpd/httpd/trunk: CHANGES support/htcacheclean.c
Posted by Graham Leggett <mi...@sharp.fm>.
Jeff Trawick wrote:
> You misunderstood my comment (possibly because I didn't write it
> clearly). What I meant by
>
> "Why not simply zap all these checks of the form
>
> if (silly user specified no-argument option again) {
> remind them who is boss
> }
>
> to avoid code bloat?"
>
> is that I didn't think it was important to check for the user specifying
> the same option more than once.
I interpreted you as saying:
zap == remove completely
"silly user specified no-argument option again" != "silly user specified
option again"
which made no sense to me at all, thus I asked for clarification.
Instead of clarification I get "Ignoring the opinion that it isn't worth
writing this code code for a moment, isn't it a sign that something
needs further work when the same exact string occurs 9 times in the code?"
The goal of the patch was to fix the fact that every error case resulted
in the same output behaviour, it wasn't to make a call as to whether an
error message was necessary or not. All 9 error cases had the same
string because a sane compiler would have optimised this to just one string.
As it turned out, I didn't remove the 9 cases, even though they are
probably overkill, because I didn't want to get beaten up by somebody
asking me why I was making two changes in one patch. Oh well.
Regards,
Graham
--
Re: svn commit: r814091 - in /httpd/httpd/trunk: CHANGES
support/htcacheclean.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Tue, Sep 15, 2009 at 1:52 PM, Graham Leggett <mi...@sharp.fm> wrote:
> Jeff Trawick wrote:
>
> > Just zap the check since it is unnecessary.
>
> So first you suggest that all the checks be collapsed down to one check,
>
I don't recall doing that, though I did suggest that if we keep the checks
we don't need to duplicate the error message 9 times ;)
> and after being asked for an example of how the one check might work,
> you're now suggesting that the checks be removed entirely?
>
You misunderstood my comment (possibly because I didn't write it clearly).
What I meant by
"Why not simply zap all these checks of the form
if (silly user specified no-argument option again) {
remind them who is boss
}
to avoid code bloat?"
is that I didn't think it was important to check for the user specifying the
same option more than once.
> > Ignoring the opinion that it isn't worth writing this code code for a
> > moment,
>
> It isn't worth writing code that makes an end user's life easier?
>
Sure; I hope we can also agree independent of this particular example that
sometimes the amount of code can exceed the real value to the end user. (I
assume we still disagree about whether or not in this particular case the
code grew to exceed its real value ;) )
Or are you suggesting that all our end users should have gdb installed,
> a copy of httpd installed with full debug symbols, and the ability to
> read C code just to find a typo on a command line?
>
What do you consider the likelihood that I was suggesting that?
You made a code change with superfluous casting, bogus duplication of error
messages, and another issue which is a matter of taste but IMO valid fodder
for discussion. Somehow I was able to raise these issues without being a
smart ass.
Re: svn commit: r814091 - in /httpd/httpd/trunk: CHANGES support/htcacheclean.c
Posted by Graham Leggett <mi...@sharp.fm>.
Jeff Trawick wrote:
> Just zap the check since it is unnecessary.
So first you suggest that all the checks be collapsed down to one check,
and after being asked for an example of how the one check might work,
you're now suggesting that the checks be removed entirely?
> Ignoring the opinion that it isn't worth writing this code code for a
> moment,
It isn't worth writing code that makes an end user's life easier?
Or are you suggesting that all our end users should have gdb installed,
a copy of httpd installed with full debug symbols, and the ability to
read C code just to find a typo on a command line?
Regards,
Graham
--
Re: svn commit: r814091 - in /httpd/httpd/trunk: CHANGES
support/htcacheclean.c
Posted by Jeff Trawick <tr...@gmail.com>.
On Tue, Sep 15, 2009 at 9:45 AM, Graham Leggett <mi...@sharp.fm> wrote:
> Jeff Trawick wrote:
>
> > Why does opt need to be cast to (int)?
>
> >From the printf man page:
>
> c The int argument is converted to an unsigned char, and the
> resulting character is written.
>
That's what happens inside printf(). On the caller side, my understanding
is that the compiler already promotes the argument to int because of the ...
.
Sanity check: Why aren't we explicitly promoting char parameters in other
calls to *printf?
> > Why not simply zap all these checks of the form
> >
> > if (silly user specified no-argument option again) {
> > remind them who is boss
> > }
> >
> > to avoid code bloat?
>
> Can you give an example?
>
Just zap the check since it is unnecessary.
Index: support/htcacheclean.c
===================================================================
--- support/htcacheclean.c (revision 815331)
+++ support/htcacheclean.c (working copy)
@@ -811,44 +811,26 @@
else {
switch (opt) {
case 'i':
- if (intelligent) {
- usage(apr_psprintf(pool, "The option '%c' cannot be
specified more than once", (int)opt));
- }
intelligent = 1;
break;
case 'D':
- if (dryrun) {
- usage(apr_psprintf(pool, "The option '%c' cannot be
specified more than once", (int)opt));
- }
dryrun = 1;
break;
case 'n':
- if (benice) {
- usage(apr_psprintf(pool, "The option '%c' cannot be
specified more than once", (int)opt));
- }
benice = 1;
break;
and so on.
--/--
Ignoring the opinion that it isn't worth writing this code code for a
moment, isn't it a sign that something needs further work when the same
exact string occurs 9 times in the code?
Re: svn commit: r814091 - in /httpd/httpd/trunk: CHANGES support/htcacheclean.c
Posted by Graham Leggett <mi...@sharp.fm>.
Jeff Trawick wrote:
> Why does opt need to be cast to (int)?
>>From the printf man page:
c The int argument is converted to an unsigned char, and the
resulting character is written.
> Why not simply zap all these checks of the form
>
> if (silly user specified no-argument option again) {
> remind them who is boss
> }
>
> to avoid code bloat?
Can you give an example?
In theory, "silly user specified no-argument option again" is one big
case statement, which in theory means we would then have two big case
statements, one after the other, and that's way more bloated than a
single string constant used over and over again (but that produces
different messages), and a single apr_psprintf() call per option.
Regards,
Graham
--