Login | Register
My pages Projects Community openCollabNet

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