Return-Path: 
 <new-httpd-return-2307-apmail-new-httpd-archive=apache.org@apache.org>
Delivered-To: apmail-new-httpd-archive@apache.org
Received: (qmail 84349 invoked by uid 500); 12 Apr 2000 17:10:55 -0000
Mailing-List: contact new-httpd-help@apache.org; run by ezmlm
Precedence: bulk
X-No-Archive: yes
Reply-To: new-httpd@apache.org
list-help: <mailto:new-httpd-help@apache.org>
list-unsubscribe: <mailto:new-httpd-unsubscribe@apache.org>
list-post: <mailto:new-httpd@apache.org>
Delivered-To: mailing list new-httpd@apache.org
Received: (qmail 84337 invoked from network); 12 Apr 2000 17:10:54 -0000
X-Authentication-Warning: koj.covalent.net: rbb owned process doing -bs
Date: Wed, 12 Apr 2000 10:12:08 -0400 (EDT)
From: rbb@covalent.net
To: new-httpd@apache.org
Subject: Re: [PATCH]  Config modules step 1
In-Reply-To: <Pine.LNX.4.10.10004111657110.11297-100000@nebula.lyra.org>
Message-ID: <Pine.LNX.4.21.0004120958380.14272-100000@koj.covalent.net>
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
X-Spam-Rating: locus.apache.org 1.6.2 0/1000/N


> The cmd_name[0] will erroneously catch the "</foo>" case.
> 
> Also, this means we have nodes in the tree that correspond to the "close"
> case. IMO, we don't want close nodes -- the tree itself describes the
> open/close semantics and close nodes have no specific data.

You're right.  I thought I had fixed that, guess not.  This won't be in
the committed code.  :-)

> 
> >...
> > #ifndef AP_CONFTREE_H
> > #define AP_CONFTREE_H
> > 
> > #include "apr_lib.h"
> 
> Not needed.

Your right.  It was at one point in the code, but it's not any longer.

> 
> > typedef struct ap_directive_t {
> >     char *directive;
> >     char *args;
> 
> "const char *" would be preferable.
> 

Agreed.  

> >...
> > 
> > #define CORE_PRIVATE
> > #include "util_cfgtree.h"
> > 
> > ap_directive_t *conf_tree;
> > 
> > ap_directive_t *ap_add_node(ap_directive_t *current, ap_directive_t *toadd, int child)
> > {
> >     if (current == NULL) {
> >         conf_tree = toadd;
> >         return conf_tree;
> >     }
> >     else if (child == 1){
> 
> No need for "else" since the primary condition has a "return"
> 

agreed.

> >         current->first_child = toadd;
> >         toadd->parent = current;
> >         toadd->first_child = toadd->next = NULL;
> >         return toadd;
> 
> The structure of this plus the caller implies that you'll end up with a
> node tree like:
> 
>   Top-level directive
>   Top-level directive
>       <Directory /foo>
>       Dir-level directive
>       Dir-level directive
>       </Directory>
>   Top-level directive
> 
> A more intuitive tree would look like:
> 
>   Top-level directive
>   Top-level directive
>   <Directory /foo>
>       Dir-level directive
>       Dir-level directive
>   Top-level directive

I actually agreed with you when I started, and about half way through, I
decided I liked this better.  The tree itself denotes the containers,
instead of the directives.  This puts everything related to a directory
container on the same level of the tree.  I'm not married to either
approach, so if you feel strongly about this let me know.  One of the
other things I like about the current approach is that we don't need to
keep an extra pointer to the current pointer.  Depending on how complex
your config is, that could be a lot of extra pointers that are really not
necessary.

consider a melding of the two approached (which is what I had meant to
code, but I forgot about the <\foobar> stuff):

    Top-level directive
    Top-level directive
        <Directory /foobar>
        Dir-level directive
        Dir-level directive
    Top-level directive

The directory container really belongs in the second level of the tree,
because it is the name of the container, just like the ServerName for the
whole server is in the top level of the tree.

I'll fix everything except the tree structure until I get a bit more
feedback on this.  

Can I assume that once all of the above issues are fixed, you are giving a
+1 to this?

Ryan

_______________________________________________________________________________
Ryan Bloom                        	rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------