kfogel@collab.net wrote:
>>+
>>+
>>+#ifndef SVN_CTYPE_H
>>+#define SVN_CTYPE_H
>>
>>
>
>At the end of this file, you have
>
> #ifdef __cplusplus
> }
> #endif /* __cplusplus */
>
>But here at the beginning, you don't have the corresponding opening
>form.
>
>
Ouch! "Never copy-and-paste at 3 in the morning". Thanks.
>>+#include <apr.h>
>>+
>>+/** Table of flags for character classification. */
>>+extern const apr_uint32_t *const svn_ctype_table;
>>+
>>+
>>+/** Check if @a c is in the character class described by @a flags.
>>+ * The @a flags is a bitwise-or combination of @t SVN_CTYPE_* *
>>+ * constants.
>>+ */
>>+#define svn_ctype_test(c, flags) \
>>+ (0 != (svn_ctype_table[(unsigned char)(c)] & (flags)))
>>
>>
>
>Just curious, why the "0 !="? Did you want to fold the boolean value
>to either 1 or 0, instead of non-zero or 0?
>
>
Basically, it's for clarity, yes.
>>+/** Check if @c is an ASCII control character. */
>>+#define svn_ctype_iscntrl(c) svn_ctype_test((c), SVN_CTYPE_CNTRL)
>>
>>
>
>Here and throughout this file, you've accidentally left out the "c"
>after "@c", thus formatting the word "is" instead.
>
Grrrr.... I always do that...
> I won't bother to
>list out all the other instances.
>
>Also, you might want to state explicitly in this header that "ASCII"
>means 7-bit ASCII, and foreshadow the UTF8 stuff coming later. I know
>this might technically be redundant, but in colloquial use, many
>people imagine a beast "ASCII" with two parts, upper and lower (i.e.,
>8-bit and 7-bit). They might wonder why we seem to ignore upper.
>
>
Well, there is no such thing as 8-bit ASCII... but I'll mention the
distinction in the @defgroup.
>>+ /* ht */ SVN_CTYPE_ASCII | SVN_CTYPE_CNTRL | SVN_CTYPE_SPACE,
>>+ /* nl */ SVN_CTYPE_ASCII | SVN_CTYPE_CNTRL | SVN_CTYPE_SPACE,
>>+ /* vt */ SVN_CTYPE_ASCII | SVN_CTYPE_CNTRL | SVN_CTYPE_SPACE,
>>+ /* np */ SVN_CTYPE_ASCII | SVN_CTYPE_CNTRL | SVN_CTYPE_SPACE,
>>+ /* cr */ SVN_CTYPE_ASCII | SVN_CTYPE_CNTRL | SVN_CTYPE_SPACE,
>>
>>
>
>We're going to remember, when it comes time to take care of that
>"#TBD" for XMLNAME and URISAFE, that VT and NP are special, right?
>
>
Oh, certainly!
>Not at all a complaint about this commit, of course. I'm sure the
>question of what officially is and isn't whitespace in ASCII was
>settled at the Council of Elrond long ago, and we can only humbly
>reflect those decisions here.
>
No, actually we get to ignore them and define our own classification
table instead. :-) If people think we shouldn't classify formfeed and
vertical tab as whitespace, we can do that, too.
The fact that we use ^L in the source files is irrelevant. :-)
> I'm just making a mental note to myself
>(well, and to this list :-) ) that for XMLNAME purposes, only three of
>these SVN_CTYPE_SPACEs are useable (plus SP below, of course).
>
>
Yes, that's why I expect to use a separate bit for XMLNAME.
>By the way, I noticed some nice UTF8 tests in the Putty code at
>
> $ svn cat svn://ixion.tartarus.org/main/putty/charset/utf8.c
>
>Looks like the license is compatible with ours, if you want to swipe
>the tests :-).
>
>
Hm, looks like those tests are for what we have in utf_validate.c. And
note that their validation code has a bug wherein they allow lead bytes
(e.g., C0) that encode a leading zero -- which aren't allowed, and which
we do check for in utf_validate.c. :-)
>
>
>
>>--- (empty file)
>>+++ trunk/subversion/libsvn_subr/genctype.py Mon Nov 29 19:56:04 2004
>>@@ -0,0 +1,93 @@
>>+#! /usr/bin/python
>>+"""getctype.py - Generate the svn_ctype character classification table.
>>+"""
>>
>>
>
>If the table was autogenerated from this file, then a comment in the C
>file ought to say so, so people realize they're dealing with generated
>content.
>
>
Good point, and thanks for the review.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Dec 2 07:50:58 2004