httpd-apreq-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Schaefer <joe+gm...@sunstarsys.com>
Subject Re: [PATCH] second snapshot of my "let's kill void*env" patch
Date Thu, 13 Jan 2005 14:19:47 GMT
Max Kellermann <max@duempel.org> writes:

>> 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.

That looks too gcc-specific IMO.  For now I think it's best 
to just leave this portion of the patch as-is, and our other 
apreq committers can give their opinion on it.  Personally I'd 
prefer we just dropped the associated warning, but I'll follow 
the general consensus here.

[regarding apreq_deconst]

>> -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.

A few points: the apr_tables we fetch things from are tables we create
and control, so we know with certainty that dropping const causes 
no harm.  The biggest problem with using apr_table_t in a public struct
is that many table operations merge values together, and those
operations are absolutely unsafe for apreq2.

If you look through the archives, we've tried to deal with this
problem years ago, first by both introducing our own apreq_table_t
object (it's still in src/).  After that petered out, I tried to 
get the necessary callback code incorporated into apr itself, which 
would have made our tables merge-safe.  They didn't commit the 
required changes.  Such is life.

That leaves us where we are today: we try to document the fact 
that our internal apr_table_t objects should not be modified by 
users.  The "deconst" issue is the very least of our concerns.


> Maybe we should better use apr_hash_t?

-1.  Preserving "document order" is a requirement, and hashes
can't do that.

> 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".

Let's leave this for another discussion.  If this is imporant
to enought to you, to try working apreq_table_t (it's really
a red-black tree with an apr_table_t like API) back into our
codebase (IIRC there are still a few unresolved bugs in there,
so it's not as stable as apr_table_t is).  I wouldn't have any 
trouble pushing that through to the perl glue for you.

>> 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..

Right.


-- 
Joe Schaefer


Mime
View raw message