httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Reid" <dr...@jetnet.co.uk>
Subject Re: [PATCH] Config modules step 1
Date Wed, 12 Apr 2000 17:23:22 GMT
+1

let's keep the momentum going on this...
----- Original Message -----
From: <rbb@covalent.net>
To: <new-httpd@apache.org>
Sent: Wednesday, April 12, 2000 3:12 PM
Subject: Re: [PATCH] Config modules step 1


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


Mime
View raw message