kfogel@collab.net writes:
> "Sergey A. Lipnevich" <sergey@optimaltec.com> writes:
> > I've applied Neon 0.24-related changes to the branch in several
> > stages. It passes all the tests (over DAV) for Neon 0.23.9, and most
> > of the tests with 0.24 (it fails 17 tests total in 8 test
> > packages). While I continue banging on remaining issues, which are
> > mostly to do with SVN properties support, I'd greatly appreciate an
> > intermediate review so that I know I'm moving forward. Could somebody
> > (MBK?) please review the neon-0.24 branch changes, at least briefly?
>
> I will take a look at these tomorrow too, Sergey. (Though I don't
> want to stop MBK or anyone else from doing so as well.)
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.
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).
Just a few suggestions:
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.
Actually, you did document it in your log message:
"new function on top of ne_xml_push_handler, adds memory pool as the
last parameter. This parameter is not used with Neon 0.23.9, but
will be with Neon 0.24."
...so what I'm saying is, that bit from the log message should be in
the code instead. As the HACKING file says:
> One should never need the log entries to understand the current
> code. If you find yourself writing a significant explanation in
> the log, you should consider carefully whether your text doesn't
> actually belong in a comment, alongside the code it explains.
(I would have said the same for the other new definitions added to
ra_dav.h in rev 6672, but I see that 6673 added the missing
explanations.)
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/.)
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.
Thanks for the "/* FIXME: Neon SSL API change */" comments -- and I'm
sure David Waite agrees :-).
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.
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.
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...
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Aug 8 19:48:43 2003