Thanks for such a thorough review! Comments below.
kfogel@collab.net wrote:
> kfogel@collab.net writes:
>
> Sergey, I've reviewed the changes so far. Nice work! I like your
> one-step-at-a-time approach very much, it makes the changes quite
> comprehensible.
Thanks :-)!
>
> My review was more about the overall shape of the changes; I haven't
> looked into the failing tests (sorry -- I hope they're just a simple
> matter of debugging).
I think they may be related to the COLLECT flag for properties elements.
Specifically, there's a single usage of it like this:
{ "", "", ELEM_unknown, SVN_RA_DAV__XML_COLLECT }
which means all unknown properties must be collected as CDATA.
So far, I don't see how Neon 0.24 interface allows for that, but I'll
keep looking. I think I'd have to bug Joe Orton about this ;-).
> In ra_dav.h, you added:
>
> +/* Push an XML handler onto Neon's handler stack */
> +void svn_ra_dav__xml_push_handler(ne_xml_parser *p,
> + const svn_ra_dav__xml_elm_t *elements,
> + svn_ra_dav__xml_validate_cb validate_cb,
> + svn_ra_dav__xml_startelm_cb startelm_cb,
> + svn_ra_dav__xml_endelm_cb endelm_cb,
> + void *userdata,
> + apr_pool_t *pool);
>
> In general, we document every argument to a function. But there can
> be exceptions, when the function is just a trivial wrapper around
> something else, which is the case here. But then, you should mention
> the something else, and explain why to use the wrapper.
Done. I must add that there were not many of such extensive comments in
ra_dav implementation ;-).
> In revision 6670, you added the 'davautocheck.sh' script. Again, the
> log message documents the script, but there is no documentation in the
> tree (not even in the script itself!). Even if you intend it to live
> only on the branch, it's best to give some explanation of what it is
> for. (By the way, if you plan to transfer it to trunk eventually,
> consider putting it in subversion/tests/.)
Done.
>
> Regarding this (from rev 6671's log message):
>
> > The replacements have been done using `perl -pi -e' command, so I
> > have to apologize if the lines got longer than 80 characters in
> > some files.
>
> No problem for the branch, but note that the long lines should be easy
> to find & fix: just run 'svn diff' and look for overly long lines :-).
> Consider doing this in the branch before you merge to trunk.
MBK fixed that for me, yeeha! I don't like this line length issue.
> Thanks for the "/* FIXME: Neon SSL API change */" comments -- and I'm
> sure David Waite agrees :-).
Sure.
> In libsvn_ra_dav/util.c, you define:
>
> +/* Finds a given element in the table of elements. If element is not found
> + * tries to find and return `unknown' element. If that is not found, returns
> + * NULL pointer. Uses a single loop to save CPU cycles. */
> +static const svn_ra_dav__xml_elm_t *lookup_elem(
> + const svn_ra_dav__xml_elm_t *table,
> + const char *nspace,
> + const char *name)
>
> Even for internal static functions, the documentation string should
> mention the arguments by name and state precisely how they are used,
> and state the return value formally (it's really "ELEM_unknown", not
> just "unknown"). The fact that it uses a single loop to save CPU
> cycles is not part of the function's interface -- a comment like that
> belongs inside the function, above the for-loop, not in the function's
> documentation.
Done.
> Similar documentation tips apply to 'shim_startelm' and all the other
> new wrappers. If they are wrappers, state what they wrap, and
> formally describe the functionality they add. You don't need to be
> verbose, just complete: supply all the information a reader needs to
> understand what the function does. For example, in the shim_*
> functions, the single most important fact is what type the 'userdata'
> argument is interpreted as.
Done.
> So I guess the whole review boils down to: documentation,
> documentation, documentation :-). But even as it is, the changes are
> pretty clear. Looking forward to the merge...
I've run the tests and I'd like to ask for an intermediate merge of the
branch except for build/buildcheck.sh.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Aug 12 01:39:00 2003