Re: cvs commit: httpd-2.0/modules/filters mod_include.c

[Greg Stein <gs...@lyra.org>]

Very cool patch!


On Sun, Sep 02, 2001 at 08:43:23AM -0000, jerenkrantz@apache.org wrote:
>...
>   +module include_module;

Hmm. That needs more stuff in there. I believe it should be:

module AP_MODULE_DECLARE_DATA include_module;

>...
>   +/* Sentinel value to store in subprocess_env for items that
>   + * shouldn't be evaluated until/unless they're actually used
>   + */
>   +static char lazy_eval_sentinel;

Please make that "static const char". Omitting the "const" means that it
goes into the process' modifiable data segment. Each time you fork, this
will contribute to another bit of working space. By making it "const", every
process shares the same const data.

[ yes, it *is* a "only" a byte of working space, but it is hard to know what
  kind of padding is occurring, and keeping it const is a good habit. ]

>...
>   +    apr_table_setn(e, "DATE_LOCAL", &lazy_eval_sentinel);
>   +    apr_table_setn(e, "DATE_GMT", &lazy_eval_sentinel);
>   +    apr_table_setn(e, "LAST_MODIFIED", &lazy_eval_sentinel);

Maybe a comment to "see add_include_vars_lazy()" so that authors will know
where to go to find what is going on, and where to go to add new lazy
variables.

I would also recommend:

#define LAZY_VALUE (&lazy_eval_sentinel)

That will clarify a good chunk of code.

>...
>                        val = apr_table_get(r->subprocess_env, start_of_var_name);
>   +                    if (val == &lazy_eval_sentinel) {
>   +                        val = add_include_vars_lazy(r, start_of_var_name);
>   +                    }
>                        *end_of_var_name = tmp_store;
>    
>    		    if (val) {
>   @@ -962,12 +1020,21 @@
>                if (!strcmp(tag, "var")) {
>                    const char *val = apr_table_get(r->subprocess_env, tag_val);
>    
>   +                if (val == &lazy_eval_sentinel) {
>   +                    val = add_include_vars_lazy(r, tag_val);
>   +                }

Two patterns of "apr_table_get, check for LAZY_VALUE, call
add_include_vars_lazy". I'd recommend a single "get_include_value()"
function to encapsulate this logic.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/