You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Kamesh Jayachandran <ka...@collab.net> on 2006/11/01 09:39:21 UTC
Re: svn commit: r22180 - trunk/subversion/libsvn_ra_serf
Few stylistic comments, please ignore if it does not make sense.
> +static date_info_t *
> +push_state(svn_ra_serf__xml_parser_t *parser,
> + date_context_t *date_ctx,
>
Unused argument 'date_ctx'.
> + date_state_e state)
> +{
> + svn_ra_serf__xml_push_state(parser, state);
> +
> + if (state == VERSION_NAME)
> + {
>
As it is called only once(and being a static function) with
state==VERSION_NAME always why to do this check again? Or this code is
futuristic?
> + date_info_t *info;
> +
> + info = apr_palloc(parser->state->pool, sizeof(*info));
> +
> + info->tmp = NULL;
> + info->tmp_len = 0;
>
Why not apr_pcalloc?
> +
> +static svn_error_t *
> +start_getdate(svn_ra_serf__xml_parser_t *parser,
> + void *userData,
> + svn_ra_serf__dav_props_t name,
> + const char **attrs)
> +{
> + date_context_t *date_ctx = userData;
>
If push_state's state is changed we don't need this date_ctx.
> + date_state_e state;
> +
> + state = parser->state->current_state;
> +
> + if (state == NONE &&
> + strcmp(name.name, "version-name") == 0)
> + {
> + push_state(parser, date_ctx, VERSION_NAME);
> + }
> +
> + return SVN_NO_ERROR;
> +}
> +
> +static svn_error_t *
> +end_getdate(svn_ra_serf__xml_parser_t *parser,
> + void *userData,
> + svn_ra_serf__dav_props_t name)
>
Arguments need to be aligned.
> +{
> +
> +static svn_error_t *
> +cdata_getdate(svn_ra_serf__xml_parser_t *parser,
> + void *userData,
> + const char *data,
> + apr_size_t len)
> +{
> + date_context_t *date_ctx = userData;
>
unused variable date_ctx.
> +
> +svn_error_t *
> +svn_ra_serf__get_dated_revision(svn_ra_session_t *ra_session,
> + svn_revnum_t *revision,
> + apr_time_t tm,
> + apr_pool_t *pool)
> +{
> + date_context_t *date_ctx;
> + svn_ra_serf__session_t *session = ra_session->priv;
> + svn_ra_serf__handler_t *handler;
> + svn_ra_serf__xml_parser_t *parser_ctx;
> + serf_bucket_t *buckets, *tmp;
>
Unused variables buckets and tmp.
> + const char *vcc_url;
> + int status_code;
> +
>
> +
> + svn_ra_serf__request_create(handler);
> +
> + *date_ctx->revision = SVN_INVALID_REVNUM;
>
Why to set with 'SVN_INVALID_REVNUM' as we don't use date_ctx down the
line and is not visible to caller?
With regards
Kamesh Jayachandran
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r22180 - trunk/subversion/libsvn_ra_serf
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 11/1/06, Kamesh Jayachandran <ka...@collab.net> wrote:
> Few stylistic comments, please ignore if it does not make sense.
Thanks for the feedback. Most of these are addressed in r22213.
> > +static date_info_t *
> > +push_state(svn_ra_serf__xml_parser_t *parser,
> > + date_context_t *date_ctx,
> >
> Unused argument 'date_ctx'.
It might be helpful to look at getlocations.c, getlocks.c, and
blame.c. They all intentionally try to follow the same structure, so
in a few cases, it leads to a slight un-usefulness in code, but I feel
the consistency between them outweighs this.
> > + date_state_e state)
> > +{
> > + svn_ra_serf__xml_push_state(parser, state);
> > +
> > + if (state == VERSION_NAME)
> > + {
> >
> As it is called only once(and being a static function) with
> state==VERSION_NAME always why to do this check again? Or this code is
> futuristic?
This is for consistency purposes.
> > + date_info_t *info;
> > +
> > + info = apr_palloc(parser->state->pool, sizeof(*info));
> > +
> > + info->tmp = NULL;
> > + info->tmp_len = 0;
> >
> Why not apr_pcalloc?
Sure. I thought about pcalloc when I wrote it, but I decided against
it. Switched.
> > +
> > +static svn_error_t *
> > +start_getdate(svn_ra_serf__xml_parser_t *parser,
> > + void *userData,
> > + svn_ra_serf__dav_props_t name,
> > + const char **attrs)
> > +{
> > + date_context_t *date_ctx = userData;
> >
> If push_state's state is changed we don't need this date_ctx.
Again, keeping for consistency purposes...
> > + date_state_e state;
> > +
> > + state = parser->state->current_state;
> > +
> > + if (state == NONE &&
> > + strcmp(name.name, "version-name") == 0)
> > + {
> > + push_state(parser, date_ctx, VERSION_NAME);
> > + }
> > +
> > + return SVN_NO_ERROR;
> > +}
> > +
> > +static svn_error_t *
> > +end_getdate(svn_ra_serf__xml_parser_t *parser,
> > + void *userData,
> > + svn_ra_serf__dav_props_t name)
> >
> Arguments need to be aligned.
Fixed.
> > +{
> > +
> > +static svn_error_t *
> > +cdata_getdate(svn_ra_serf__xml_parser_t *parser,
> > + void *userData,
> > + const char *data,
> > + apr_size_t len)
> > +{
> > + date_context_t *date_ctx = userData;
> >
> unused variable date_ctx.
Again, consistency. =)
> > +svn_error_t *
> > +svn_ra_serf__get_dated_revision(svn_ra_session_t *ra_session,
> > + svn_revnum_t *revision,
> > + apr_time_t tm,
> > + apr_pool_t *pool)
> > +{
> > + date_context_t *date_ctx;
> > + svn_ra_serf__session_t *session = ra_session->priv;
> > + svn_ra_serf__handler_t *handler;
> > + svn_ra_serf__xml_parser_t *parser_ctx;
> > + serf_bucket_t *buckets, *tmp;
> >
> Unused variables buckets and tmp.
Fixed.
> > + const char *vcc_url;
> > + int status_code;
> > +
> >
> > +
> > + svn_ra_serf__request_create(handler);
> > +
> > + *date_ctx->revision = SVN_INVALID_REVNUM;
> >
> Why to set with 'SVN_INVALID_REVNUM' as we don't use date_ctx down the
> line and is not visible to caller?
The code is a bit clever. Take a look at end_getdate() circa line
125. This line you pointed out initializes the return revision
pointer to be invalid before we run our context - only if we receive
the version-name field in the XML response in end_getdate(), do we
update date_ctx->revision pointer. Which gets passed back to the
caller...
Hope this helps. Thanks! -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org