On 11/1/06, Kamesh Jayachandran <kamesh@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
Received on Sat Nov 4 17:06:45 2006