On 3/18/06, Julian Foad <julianfoad@btopenworld.com> wrote:
> Malcolm Rowe wrote:
> > On Fri, Mar 17, 2006 at 08:32:52PM +0100, Peter N. Lundblad wrote:
>
> Re. svn_xml_is_xml_name():
>
> >>Hmmm, would it be better to document the ASCII part as a @todo or @bug?
> >>That would allow us to extend this function to cover the whole Name
> >>production when someone gets around to it.
> >
> > I don't know whether the ASCII part is intended or not - and in any case,
> > it would seem a little unreasonable to change the function so drastically.
> > Maybe.
> >
> > The checkin that I did was just a clarification to the docstring zhakov
> > introduced in r18202. Ah, I see that Julian originally introduced this
> > function in r7528 - maybe he has an opinion as to what the contract
> > should actually be, now that the function is public?
>
> 1. Re. semantics: For this to be a public API, your new doc string is a good
> description of the implementation, and these are acceptable semantics for a
> public API, but its name ought to reflect the ASCII aspect of its semantics.
>
> At some later stage we're bound to want an "is_xml_name" with full UTF-8
> semantics, and I don't think it's OK to later change the meaning that much.
>
> Therefore let's name it something like "svn_xml_is_ascii_xml_name".
>
I completly agree with proposed function name.
>
> 2. Re. r18202: that commit purports to avoid sharing variables
> (svn_ctype_table) between libraries:
>
> > ------------------------------------------------------------------------
> > r18202 | zhakov | 2006-01-24 14:12:31 +0000 (Tue, 24 Jan 2006) | 13 lines
> > Changed paths:
> > M /trunk/subversion/include/svn_xml.h
> > M /trunk/subversion/libsvn_client/prop_commands.c
> > M /trunk/subversion/libsvn_subr/xml.c
> >
> > Factor out svn_xml_is_xml_name_valid() to eliminate usage of
> > svn_ctype_table variable outside of libsvn_subr library. This needed because
> > exporting variables from Windows DLLs requires __declspec(dllexport).
> > See disscussion here: http://svn.haxx.se/dev/archive-2005-12/0178.shtml
> >
> > * subversion/include/svn_xml.h
> > * subversion/libsvn_subr/xml.c
> > (svn_xml_is_xml_name_valid): New function, factored out from
> > is_valid_prop_name()
> >
> > * subversion/libsvn_client/prop_commands.c:
> > (is_valid_prop_name): Use new function svn_xml_is_xml_name_valid()
> >
> > ------------------------------------------------------------------------
> > Index: subversion/include/svn_xml.h
> > ===================================================================
> [...]
> > +/** Check whether the UTF8 NAME is a the ASCII subset of an XML "Name".
> > + * XML "Name" is defined at http://www.w3.org/TR/REC-xml#sec-common-syn
> > + *
> > + * @since New in 1.4.
> > + */
> > +svn_boolean_t svn_xml_is_xml_name_valid (const char *name);
>
> That commit eliminated the single use of an svn_cytpe macro outside libsvn_subr
> at the time, but did nothing to prevent such use. The email discussion said
> that public access to svn_ctype_table[] was to be deprecated, which would
> require the svn_ctype_* macros to be converted to functions, but that has not
> been done. Now there are other uses outside libsvn_subr, so presumably the
> Win32 DLLs again don't build/work properly - is that the case?
>
> I think that commit wasn't a proper way to solve the problem. If we solve the
> variable-sharing problem properly, then that commit r18202 can be reverted, and
> the problem of how to name and describe the XML-name function goes away. That
> would be good because we've no reason to want such a function in our public
> API, as far as I know.
>
At the moment when I commited this change, it was only one usage of
svn_ctype out of libsvn_subr module. I was going to deprecated
svn_ctype_table, but this requires function call for each access,
which could decrease performance.
Anyway I didn't see reasons why we cannot leave
svn_xml_is_ascii_xml_name in our public API? We already have function
svn_xml_is_xml_safe().
I don't press for keep it, so revert my commit if you consider.
--
Ivan Zhakov
Received on Mon Mar 20 14:15:36 2006