Julian Foad wrote on Fri, Aug 20, 2010 at 18:12:29 +0100:
> On Fri, 2010-08-20, Daniel Shahaf wrote:
> > I arrived at these docstrings by reverse-engineering the implementation
> > and some guesses. Could someone confirm my understanding?
> >
> > [[[
> > * subversion/libsvn_ra_neon/util.c
> > (multistatus_elements, multistatus_nesting_table, validate_element):
> > Add docstrings.
> > ]]]
> >
> > [[[
> > Index: subversion/libsvn_ra_neon/util.c
> > ===================================================================
> > --- subversion/libsvn_ra_neon/util.c (revision 987515)
> > +++ subversion/libsvn_ra_neon/util.c (working copy)
> > @@ -82,6 +82,7 @@
> > * for our custom error parser - could use the ne_basic.h interfaces.
> > */
> >
> > +/* List of XML elements expected in 207 Multi-Status responses. */
> > static const svn_ra_neon__xml_elm_t multistatus_elements[] =
> > { { "DAV:", "multistatus", ELEM_multistatus, 0 },
> > { "DAV:", "response", ELEM_response, 0 },
> > @@ -99,6 +100,14 @@
> > };
> >
> >
> > +/* Sparse matrix listing the permitted child elements of each element.
> > +
> > + The permitted direct children of the element named in the first column
> > are
> > + the elements named in the remainder of the row.
> > +
> > + There may be any number of rows, and any number of columns in each row;
> > any
> > + non-positive value (such as SVN_RA_NEON__XML_INVALID) serves as a
> > sentinel.
> > + */
> > static const int multistatus_nesting_table[][5] =
> > { { ELEM_root, ELEM_multistatus, SVN_RA_NEON__XML_INVALID },
> > { ELEM_multistatus, ELEM_response, ELEM_responsedescription,
> > @@ -115,6 +124,9 @@
> > };
> >
> >
> > +/* PARENT and CHILD are enum values of ELEM_* type.
> > + Return a positive integer if CHILD is a valid direct child of PARENT,
> > + and a negative integer otherwise. */
>
> From my own peering at it, there's a bit more to it: if not a valid
> child, it returns specifically the status listed in the table for that
> parent, INVALID for some parent elements and DECLINE for others. I
> assume that is important.
>
Good point. Adjusted the patch accordingly.
> > static int
> > validate_element(int parent, int child)
> > {
> > ]]]
>
> I notice that there is a type 'svn_ra_neon__xml_elmid', typedef'd to
> 'int', which is used in some places (but far from all places) for enum
> values of ELEM_* type. Whether 'svn_ra_neon__xml_elmid' is intended to
> be used for non-element-id status values (INVALID, DECLINE) as well, I
> have no idea.
>
> The actual argument passed as 'child' is of that data type. Changing
> the function prototype to use that type instead of 'int' for 'child'
> would give valuable information. Tracing the origin of 'parent', I
> *think* logically it has to be of that type too - in other words a valid
> element enum value rather than a status code, but it arrives as 'int'.
> But it's probably not a good idea to change the data type in just one
> place; if anyone is going to do that they should figure out the overall
> pattern and do it consistently.
>
Yeah. And also name the currently-anonymous ELEM_* enum type. I've
added a ###-comment along these lines to the patch.
> Also I can deduce that start_207_element(), which calls
> validate_element(), implements svn_ra_neon__startelm_cb_t. I'll state
> that in a doc string because it usefully leads to documentation about
> its inputs and outputs. Similarly for end_207_element().
>
> And *THANK YOU*.
>
> - Julian
>
>
Thanks for your help. Committed r987939; further reviews, tweaks, etc,
are welcome.
