httpd-apreq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Max Kellermann <...@duempel.org>
Subject Re: [PATCH] second snapshot of my "let's kill void*env" patch
Date Thu, 13 Jan 2005 08:45:58 GMT
On 2005/01/13 05:26, Joe Schaefer <joe+gmane@sunstarsys.com> wrote:
> Also, will older (or non-gcc) compilers understand this 
> "-isystem" stuff?
> 
> -                APACHE2_INCLUDES=-I`$APACHE2_APXS -q INCLUDEDIR`
> +                APACHE2_INCLUDES="-isystem `$APACHE2_APXS -q INCLUDEDIR`"

Probably not. That is a workaround for warnings in include files
beyond my control. I hard coded that here to get rid of all
warnings now, and repair build scripts later.

> To my knowledge, noop statements like this
> 
>     +    (void)d;
> 
> aren't found within httpd/apr's core source code, so I don't 
> think it's a good idea to start introducing that construct 
> into ours.  Do you know if gcc recognizes a variable name 
> (like "dummy" maybe) that will tell it to overlook unused 
> function arguments?

Yes, there is. That was the "quick" fix, the real solution is the
__attribute__((unused)) flag in gcc. I will put that on a conditional
macro for my next patch.

> -1.  This looks a bit too pedantic for my tastes.  Instead of
> introducing this, how about we try to adjust the few places that
> cast away the const qualifier?  It looks like we're only doing that
> when dealing with apreq_value_t objects or iovecs which we have
> control over.

I believe it's the least pedantic bit in my warning fixes :) Casting
the const away is a Bad Thing(TM). We need it here to cast a const
char returned by apr_table_get to an apreq_value_t. IMHO, the
apreq_strtoval() function should go away, it's heavily unsafe. The
root of the problem is the apr_table_t only accepting a const string
as value.

Maybe we should better use apr_hash_t? We can wrap that in (inline)
functions for convenience. The "give me parameter as string" should
only be a special case of the general "give me parameter as
apreq_value_t".

> I have mixed feelings about gcc's inline concept.  IMO we should
> either leave these as macros or use APR_INLINE and support them as
> ordinary functions.

On gcc with optimizations enabled, there should be no penalty if you
convert macros to (small) inline functions. Of course, for compiler
compatibility, we should use APR_INLINE..

> > From my TODO list:
> > - still, more testing and test cases, especially for subrequests
>                                                        ^^^^^^^^^^^
> 
> I still don't understand how "subrequests" work with the existing
> code, so I'm not so concerned about any subrequest breakage wrt 
> mod_apreq (internal redirects are another story, but our existing
> tests should flesh most of that out).  If it's the "local vs. 
> global apreq_request_t" stuff you are referring to here, then I 
> think the pending API changes (dropping the secondary NULL argument 
> from apreq_request and apreq_jar) will sort that out well enough.

No, it's handling of apache subrequests in the mod_apreq filter
code. Not the old (obsolete) secondary apreq_request_t instance.

Max


Mime
View raw message