[
Date Prev][
Date Next][
Thread Prev][
Thread Next][
Date Index][
Thread Index]
[Catacomb] Re: A patch for cadaver-dasl
Hi Sung,
This is looking good - I have a few comments in-lined from a brief code
review.
On Mon, Aug 26, 2002 at 08:51:00PM -0700, Sung Kim wrote:
> Hello Joe,
>
> This is a DASL patch for cadaver. In this patch, we didn't change neon
> at all. We just added search module in cadaver on the top of neon
> library like subversion.
OK.
> The functions of the patch can be summarized as those:
>
> - search command added
> - parser for the search query and generate DASL query
> - parser DASL response and display
> - several set values for search (The variable name is not sweet. :-)
> * searchall : weather search and display all dead props
> * searchdepth : depth for search
> * searchorder : prop(s) for ascending order
> * searchdorder : prop(s) for descending order
>
> The search.c includes all search module including parser, so it seems
> a bit big.
> Maybe we can separate it to two files: one for search main module,
> the other for search parser.
That is definitely a good idea.
> +++ search.c Mon Aug 26 20:23:05 2002
> @@ -0,0 +1,1423 @@
> +/*
> + 'search' for cadaver
> + Copyright (C) 2000-2001, Joe Orton <joe@manyfish.co.uk>,
I'm still not really happy about this, since I didn't write the code.
Without a formal copyright assignment I'd prefer you owned the copyright
(or UCSC, or whatever).
> +typedef struct dead_prop_t {
POSIX owns the "*_t" namespace, I'd prefer to not invade that... you
probably don't need the tag name for this typedef anyway, so can you
just drop the "dead_prop_t" out of this declaration, and similarly with
resource_t.
Declaring a "resource" or a "struct resource_t" is also confusing, since
cadaver.h already defines a "struct resource", I'm not sure what can be
done about that though.
> + case ELEM_href : /* href */
> + if(sctx->curr==NULL)
> + set_xml_error(sctx, "XML : </%s> is in the wrong place", elm->name);
...
> + if(sctx->curr==NULL)
> + set_xml_error(sctx, "XML : </%s> is in the wrong place", elm->name);
...
> + if(sctx->curr==NULL)
> + set_xml_error(sctx, "XML : </%s> is in the wrong place", elm->name);
Way too much duplicated code in there!
> +/* Custom function of type ne_accept_response. */
> +static int search_accepter(void *userdata,
> + ne_request *req,
> + const ne_status *st)
> +{
> + return (st->code == 207);
> +}
This is equivalent to ne_accept_207 from ne_207.h.
