Re: svn commit: r987592 - /subversion/trunk/subversion/libsvn_ra_neon/util.c

[Greg Stein <gs...@gmail.com>]

On Fri, Aug 20, 2010 at 13:58,  <ju...@apache.org> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_ra_neon/util.c Fri Aug 20 17:58:10 2010
>...
> -static ne_xml_parser *
> +/* Create a status parser attached to the request REQ.  Detected errors
> +   will be returned there. */
> +static void
>  multistatus_parser_create(svn_ra_neon__request_t *req)
>  {
>   multistatus_baton_t *b = apr_pcalloc(req->pool, sizeof(*b));
> -  ne_xml_parser *multistatus_parser =
> -    svn_ra_neon__xml_parser_create(req, ne_accept_207,
> -                                   start_207_element,
> -                                   svn_ra_neon__xml_collect_cdata,
> -                                   end_207_element, b);
> +
> +  svn_ra_neon__xml_parser_create(req, ne_accept_207,

Woah. You should have a (void) cast in there and (or?) a comment
noting that the return value is not used. Otherwise, it looks like a
programming error.

>...

Cheers,
-g

Re: svn commit: r987592 - /subversion/trunk/subversion/libsvn_ra_neon/util.c

[Julian Foad <ju...@wandisco.com>]

On Fri, 2010-08-20, Greg Stein wrote:
> On Fri, Aug 20, 2010 at 13:58,  <ju...@apache.org> wrote:
> >...
> > +++ subversion/trunk/subversion/libsvn_ra_neon/util.c Fri Aug 20 17:58:10 2010
> >...
> > -static ne_xml_parser *
> > +/* Create a status parser attached to the request REQ.  Detected errors
> > +   will be returned there. */
> > +static void
> >  multistatus_parser_create(svn_ra_neon__request_t *req)
> >  {
> >   multistatus_baton_t *b = apr_pcalloc(req->pool, sizeof(*b));
> > -  ne_xml_parser *multistatus_parser =
> > -    svn_ra_neon__xml_parser_create(req, ne_accept_207,
> > -                                   start_207_element,
> > -                                   svn_ra_neon__xml_collect_cdata,
> > -                                   end_207_element, b);
> > +
> > +  svn_ra_neon__xml_parser_create(req, ne_accept_207,
> 
> Woah. You should have a (void) cast in there and (or?) a comment
> noting that the return value is not used. Otherwise, it looks like a
> programming error.

If you know that the called function attaches the parser to REQ, it
doesn't look so odd.  Its doc string didn't say so, so I've tweaked it.
I also added a comment here at the point of call for those who still
think it looks odd.  (I didn't add a cast to void, as ignoring return
values is a standard idiom in C yet that would make it stand out like a
sore thumb.)

I also wrote a doc string for static function attach_ne_body_reader(),
since it's involved here too.

r988068.

I did notice that all the other callers seem to keep a note of the
returned parser, and use it later in a call to
svn_ra_neon__check_parse_error().  I suppose there's something special
about this particular caller, and it doesn't need to do that.  I'm just
mentioning this in case anyone thinks it's wrong; otherwise I'm assuming
it's as intended.

- Julian