Charles Bailey <bailey.charles@gmail.com> writes:
> On 6/20/05, Philip Martin <philip@codematters.co.uk> wrote:
>> Charles Bailey <bailey.charles@gmail.com> writes:
>>
>> > --- /dev/null Mon Jun 6 11:06:27 2005
>> > +++ subversion/libsvn_subr/escape.c Fri Jun 3 19:16:09 2005
>>
>> New files need a copyright notice.
>
> Right. I'd noticed that, but since I don't know Collabnet's policy
> here I didn't want
> to presume. Is it acceptable to clone the copyright from an existing
> file? Does
> that suffice to Collabnet as assignment of copyright?
I think so.
> Do I have a pledge from
> Collabnet that any code whose copyright is assigned in this way will
> remain freely
> available in perpetuity? (By "freely available", I mean terms
> substantially identical
> to Subversion's current licensing; I'm not trying to start a skirmish
> over software
> licensing.)
I can't speak for CollabNet, but I'll point out that the Subversion
licence is not a copyleft licence and so anyone, including CollabNet,
can make a proprietary version.
[...]
>> The patch is mangled here, and in several other places.
>
> Right. When Michael Thelen pointed that out, I resent it as an attachment;
> I hope that came through in better shape.
I had one other copy in a forwarded message, it was also mangled.
[...]
>> Those big macros also make me uneasy.
>
> Is the concern that it'll choke come compilers, or that they're hard
> to maintain?
I don't think compilers will have a problem, I think it makes it
harder for humans.
> This may just be a stylistic issue: I tend to favor macros for
> repeated tasks, on the theory that as long as one debugs it carefully,
> the benefit of inlining is worth the effort of coding.
The macros are only used once in your patch...
[...]
>> > -fuzzy_escape (const char *src, apr_size_t len, apr_pool_t *pool)
>> > +ascii_fuzzy_escape (const char *src, apr_size_t len, apr_pool_t *pool)
>> > {
>> > - const char *src_orig = src, *src_end = src + len;
>> > - apr_size_t new_len = 0;
>> > - char *new;
>> > - const char *new_orig;
>> > + static unsigned char asciinonul[256];
>> > + svn_stringbuf_t *result = NULL;
>> >
>> > - /* First count how big a dest string we'll need. */
>> > - while (src < src_end)
>> > - {
>> > - if (! svn_ctype_isascii (*src) || *src == '\0')
>> > - new_len += 5; /* 5 slots, for "?\XXX" */
>> > - else
>> > - new_len += 1; /* one slot for the 7-bit char */
>> > + if (!asciinonul[0]) {
>> > + asciinonul[0] = 255; /* NUL's not allowed */
>>
>> That doesn't look threadsafe.
>
> It is safe, though it doesn't prevent duplication.
No, it's not safe. Consider:
- asciinonul is static initialised to zero
- thread 1 tests asciinonul[0] and gets zero
- thread 1 sets asciinonul[0] to 255, the rest of asciinonul is still zero
- thread 2 tests asciinonul[0] and gets 255
- thread 2 uses the rest of asciinonul although it is still zero, oops!
You could try to fix it by changing the code to set asciinonul[0]
later, but that's still not threadsafe:
- An optimising compiler can reorder the memory writes.
- The CPU pipeline can reorder the memory writes.
- Even if the writes are not reordered, on a multi-CPU system there is
no guarantee that writes by one CPU will become visible to another CPU
in the same order.
[...]
>> Overall this patch makes me uneasy, I'd prefer a patch that builds on
>> our existing ctype and/or utf-8 code. However as I haven't tried to
>> implement such a patch I don't know whether our existing code is
>> a suitable starting point.
>
> That's certainly possible. I'd actually started out that way, and
> switched to the above when it seemed I was about to add ya parallel
> routine to escape a string in a slightly different way.
>
> Thanks for the review. If you or one of the other core developers
> could answer some of the "philosophic" questions above, I can revise
> the patch appropriately.
Well, that's a bit tricky. Your patch looked overly complicated to
me, but then I haven't tried to write an alternative so I don't know
whether there is a better way.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jun 22 02:33:41 2005