You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2008/03/31 22:24:50 UTC

Re: svn commit: r642971 - in /httpd/httpd/trunk: include/ap_expr.h include/ap_mmn.h server/main.c server/util_expr.c


On 03/31/2008 02:17 PM, niq@apache.org wrote:
> Author: niq
> Date: Mon Mar 31 05:16:58 2008
> New Revision: 642971
> 
> URL: http://svn.apache.org/viewvc?rev=642971&view=rev
> Log:
> Flesh out ap_expr with:
>  * Re-usable parse trees
>  * Canonical string parser function (candidate)
> 
> Modified:
>     httpd/httpd/trunk/include/ap_expr.h
>     httpd/httpd/trunk/include/ap_mmn.h
>     httpd/httpd/trunk/server/main.c
>     httpd/httpd/trunk/server/util_expr.c
> 
> Modified: httpd/httpd/trunk/include/ap_expr.h
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/include/ap_expr.h?rev=642971&r1=642970&r2=642971&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/include/ap_expr.h (original)
> +++ httpd/httpd/trunk/include/ap_expr.h Mon Mar 31 05:16:58 2008

> @@ -113,6 +116,43 @@
>                                     int *was_error, backref_t **reptr,
>                                     string_func_t string_func,
>                                     opt_func_t eval_func);
> +
> +/**
> + * Internal initialisation of ap_expr (for httpd)
> + * @param pool Pool
> + * @return APR_SUCCESS or error
> + */
> +AP_DECLARE(apr_status_t) ap_expr_init(apr_pool_t *pool);
> +
> +/**
> + * Default string evaluation function for passing to ap_expr_eval and
> + * ap_expr_evalstring.  Use this (and update as necessary) to offer
> + * a consistent expression syntax across different modules.
> + * Supports the following:
> + *     $req{foo}     - request header "foo"
> + *     $resp{foo}    - response header "foo"
> + *     $env{foo}     - environment variable "foo"
> + *     $handler      - r->handler
> + *     $content-type - r->content_type
> + * Other strings are returned unmodified.
> + * @param r The current request
> + * @param str The string to evaluate
> + * @return The evaluated string
> + */
> +AP_DECLARE(const char*) ap_expr_string(request_rec *r, const char *str);
> +
> +/**
> + * Clone a parse tree.  This is required if you create a parse tree
> + * using ap_expr_parse, and wish to re-use it many times in ap_expr_eval.
> + * It is not required if you need to use it just once.
> + * @param pool Pool
> + * @param node The parse tree to clone
> + * @param parent Parent node (for internal use when recursing - pass in NULL)

I don't like exposing internals to a public API. If the API user always calls
it with NULL we should hide this from the API users by using a thin wrapper.


> Modified: httpd/httpd/trunk/server/util_expr.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util_expr.c?rev=642971&r1=642970&r2=642971&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/util_expr.c (original)
> +++ httpd/httpd/trunk/server/util_expr.c Mon Mar 31 05:16:58 2008
> @@ -26,6 +26,7 @@
>  #include "http_log.h"
>  
>  #include "ap_expr.h"
> +#include <assert.h>

Are we sure that assert.h is available on all platforms?

>  #if 1
>  /*
>   * +-------------------------------------------------------+
> @@ -43,10 +44,7 @@
>  } while(0)
>  
>  #define CREATE_NODE(pool,name) do {                          \
> -    (name) = apr_palloc(pool, sizeof(*(name)));      \
> -    (name)->parent = (name)->left = (name)->right = NULL; \
> -    (name)->done = 0;                                     \
> -    (name)->dump_done = 0;   

Why removing this initializations?

                              \
> +    (name) = apr_pcalloc(pool, sizeof(*(name)));
>  } while(0)
>  
>  static void debug_printf(request_rec *r, const char *fmt, ...)


Regards

Rüdiger

Re: svn commit: r642971 - in /httpd/httpd/trunk: include/ap_expr.h include/ap_mmn.h server/main.c server/util_expr.c

Posted by André Malo <nd...@perlig.de>.
* Nick Kew wrote:

> On Mon, 31 Mar 2008 22:24:50 +0200
>
> Ruediger Pluem <rp...@apache.org> wrote:

> > >  #define CREATE_NODE(pool,name) do {                          \
> > > -    (name) = apr_palloc(pool, sizeof(*(name)));      \
> > > -    (name)->parent = (name)->left = (name)->right = NULL; \
> > > -    (name)->done = 0;                                     \
> > > -    (name)->dump_done = 0;
> >
> > Why removing this initializations?
> >
> >                               \
> >
> > > +    (name) = apr_pcalloc(pool, sizeof(*(name)));
>
> apr_pcalloc is a more concise equivalent:-)

Really? Maybe I'm too theoretical here, but what happens if NULL != (int)0?

nd
-- 
>kann mir jemand sagen, was genau @-Domains sind?
Ein Mythos. Ein Werbetrick. Verarsche. Nenn es wie du willst...

                 -- Alexandra Buss und Björn Höhrmann in dciwam

Re: svn commit: r642971 - in /httpd/httpd/trunk: include/ap_expr.h include/ap_mmn.h server/main.c server/util_expr.c

Posted by Nick Kew <ni...@webthing.com>.
On Mon, 31 Mar 2008 22:24:50 +0200
Ruediger Pluem <rp...@apache.org> wrote:

> I don't like exposing internals to a public API. If the API user
> always calls it with NULL we should hide this from the API users by
> using a thin wrapper.

Indeed, I think clone needs to be hidden altogether, and plan to do so
(make it automatic when parse and eval are called separately).

> >  #include "ap_expr.h"
> > +#include <assert.h>
> 
> Are we sure that assert.h is available on all platforms?

Hmmm.

I thought we had an assert function, but failed to find apr_assert.
Turns out it's ap_assert, so I'll change it to that.

Thanks for that.

> >  #define CREATE_NODE(pool,name) do {                          \
> > -    (name) = apr_palloc(pool, sizeof(*(name)));      \
> > -    (name)->parent = (name)->left = (name)->right = NULL; \
> > -    (name)->done = 0;                                     \
> > -    (name)->dump_done = 0;   
> 
> Why removing this initializations?
> 
>                               \
> > +    (name) = apr_pcalloc(pool, sizeof(*(name)));

apr_pcalloc is a more concise equivalent:-)

I changed the macro when I started hacking a solution that
would have changed the parse_node struct.  That was before
resorting to the path of least resistance with clone().

-- 
Nick Kew

Application Development with Apache - the Apache Modules Book
http://www.apachetutor.org/