Mailing-List: contact apreq-dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Received-SPF: pass (hermes.apache.org: domain of gcaa-apreq-dev@m.gmane.org
 designates 80.91.229.2 as permitted sender)
Mail-Followup-To: apreq-dev@httpd.apache.org
To: apreq-dev@httpd.apache.org
From: Joe Schaefer <joe+gmane@sunstarsys.com>
Subject: Re: [PATCH] second snapshot of my "let's kill void*env" patch
Date: Wed, 12 Jan 2005 23:26:37 -0500
Lines: 127
Message-ID: <87vfa2ymwy.fsf@gemini.sunstarsys.com>
References: <20050112221409.GA20209@roonstrasse.net>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Mail-Copies-To: never
User-Agent: Gnus/5.1007 (Gnus v5.10.7) Emacs/21.3 (gnu/linux)
Cancel-Lock: sha1:FxZ9kmO88GF5dmzRDcvHAGu7Kfw=
Sender: news <news@sea.gmane.org>

Max Kellermann <max@duempel.org> writes:

> Hi,
>
> the patches have become too large to be accepted by ezmlm, please get
> them from my web server:
>
>  http://max.kellermann.name/download/apache/apreq/lets-kill-void-env/

Excellent!  After a quick read, I must say I like the patches.
There are a few issues that need discussion, but for now let's 
focus on the simpler stuff ...

> Changes since the first snapshot:
>
> apreq-fix_gcc_warnings-v2.patch changes

We still support ancient ansi C compilers and non-gcc platforms, 
so IMO using the -std=c99 flag isn't appropriate for us yet.
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`"


> - fixed variable declaration after statement in
>   apreq_create_dir_config()

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?  If not, maybe we need to scale back 
the argument flags a bit.

> - fixed many many warnings in the test cases
> - warning workaround in env/t/TEST.PL

Cool!

> - apreq.h: introduce apreq_deconst() as a hack

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

> - apreq.h: converted some macros to inline functions to avoid name
>   clashes (warnings)

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.


> new: apreq-move_cgi.patch
> - move the cgi env module to its own source

+1

> new: apreq-rename_to_apreq_env_module_t.patch
> - rename apreq_env_t to apreq_env_module_t

+1

> apreq-lets_kill_void_env-0.0.2.patch changes
> - apreq_handle_t renamed to apreq_env_handle_t
> - src/apreq_apache.h moved to env/apreq_env_apache2.h
> - constructors renamed to areqp_env_make_X()
> - renamed "null" module to "custom"
> - moved "custom" module into apreq_env_custom.c
> - "custom" accepts a bucket brigade
> - tests use custom+bb for MFD tests

Looks good to me so far, but I haven't had a chance
to dig into it in detail yet.  Hopefully sometime
tomorrow I'll be able to say more.

[...]

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

> - perl glue (joe)

No problem so far.

> - remove the second parameter from apreq_request() and apreq_cookie()

+1.

> - review the apreq_env_t interface

Will do.

> - more apreq cleanup
>   - redesign apreq_parser_initialize() in a thread safe way

+1

>   - some strings go through apr_strdup() twice, e.g. in
>     apreq_env_temp_dir()

+1

> - check build scripts with other gcc versions or other c compilers

Yup.  Looks like the right plan!

-- 
Joe Schaefer