httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sander Striker" <stri...@apache.org>
Subject RE: Code questions
Date Thu, 07 Mar 2002 19:48:27 GMT
> From: Ryan Bloom [mailto:rbb@covalent.net]
> Sent: 07 March 2002 19:58

>> server/config.c:396
>>     return !!(cmd->limited & (AP_METHOD_BIT << methnum));
>>            ^^
>> 
>> Is that a typo or intentional?
> 
> It's intentional.  This line always sparks a VERY large debate.

Then why didn't any one leave a nice comment behind so that this
doesn't happen again? ;)

> The reason for it is that it is the only way to ensure that you have a 1 or
> 0 result.  By negating twice, the first negation always takes the result
> to 0 or 1, and second always gives the opposite result. 

It's not exactly the only way.  There are two more:

1)    return (cmd->limited & (AP_METHOD_BIT << methnum)) ? 1 : 0;

2)    return (cmd->limited & (AP_METHOD_BIT << methnum)) != 0;

And method 3, this entire block:

    /*
     * A method number either hardcoded into apache or
     * added by a module and registered.
     */
    if (methnum != M_INVALID) {
        return (cmd->limited & (AP_METHOD_BIT << methnum)) ? 1 : 0;
    }

    return 0; /* not found */

can be written as:

    /*
     * A method number either hardcoded into apache or
     * added by a module and registered.
     */
    return (methnum != M_INVALID
            && (cmd->limited & (AP_METHOD_BIT << methnum)));


If noone steps up I'll change it to method 1, since that is
most clear to read for everyone I think.

>> server/core.c:661
>>     AP_DECLARE(const char *) ap_document_root(request_rec *r) /* Don't use this!
*/
>> 
>> If we shouldn't use it, why is it still here?
> 
> Because people are lazy and most people didn't realize that comment
> existed.  If nobody is using that function, remove it.

Okay, thanks for the heads up.
 
>> server/core.c:691
>>     /* Should probably just get rid of this... the only code that cares is
>>      * part of the core anyway (and in fact, it isn't publicised to other
>>      * modules).
>>      */
>> 
>> Read the comment.
> 
> Check to make sure nobody uses it, and remove it if nobody does.

Ok.
 
>> server/listen.c:320
>>     /*free(lr);*/
>> 
>> Can this go?  Was there a future purpose to this call,
>> or was it just old code commented out?
> 
> It most likely highlights a memory leak more than anything else.  These
> structures use to be malloc'ed, and free'd at the correct times.  Now we
> use palloc to allocate them.  I would bet that the free was left so that
> somebody would remember to look for the leak.

Consider the line torched.
 
> Ryan

Thanks,

Sander


Mime
View raw message