Re: [Libvir] [PATCH] Enhance virBuffer code
On Fri, Dec 14, 2007 at 11:53:35PM +0100, Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)redhat.com> wrote:
> On Fri, Dec 14, 2007 at 08:34:13AM +0100, Jim Meyering wrote:
>> Daniel Veillard <veillard(a)redhat.com> wrote:
>> > On Thu, Dec 13, 2007 at 11:21:31PM +0100, Jim Meyering wrote:
>> >> "Richard W.M. Jones" <rjones(a)redhat.com> wrote:
>> >> ...
>> >> > + * virBufferURIEncodeString:
>> >> > + * @buf: the buffer to append to
>> >> > + * @str: the string argument which will be URI-encoded
>> >> > + *
>> >> > + * Append the string to the buffer. The string will be
URI-encoded
>> >> > + * during the append (ie any non alpha-numeric characters are
replaced
>> >> > + * with '%xx' hex sequences).
>> >> > + *
>> >> > + * Returns 0 successful, -1 in case of internal or API error.
>> >> > + */
>> >> > +int
>> >> > +virBufferURIEncodeString (virBufferPtr buf, const char *str)
>> >> > +{
>> >> > + int grow_size = 0;
>> >> > + const char *p;
>> >> > + unsigned char uc;
>> >> > + const char *hex = "0123456789abcdef";
>> >> > +
>> >> > + for (p = str; *p; ++p) {
>> >> > + /* Want to leave only strict 7 bit ASCII alphanumerics
... */
>> >> > + if ((*p >= '0' && *p <=
'9') ||
>> >> > + (*p >= 'a' && *p <=
'z') ||
>> >> > + (*p >= 'A' && *p <=
'Z'))
>> >> ...
>> >> > + for (p = str; *p; ++p) {
>> >> > + if ((*p >= '0' && *p <=
'9') ||
>> >> > + (*p >= 'a' && *p <=
'z') ||
>> >> > + (*p >= 'A' && *p <=
'Z'))
>> >>
>> >> Hi Rich,
>> >>
>> >> What do you think of using this?
>> >>
>> >> isascii (*p) && isalnum (*p)
>> >
>> > I have learned to be very cautious of the is* macros because they
>> > tend to be local dependant whichis usually really not what you would
>> > like or expect. In that case this may work, but explicit ranges
>> > are 100% clear about what you intend to accept or not, that's why I
>> > usually prefer them.
>>
>> You're right that we shouldn't use isalnum here.
>> However, we shouldn't use inequality comparisons, either.
>> While 0 <= c <= 9 is guaranteed to be ok for the digits, the
>> a..z and A..Z ranges need not be contiguous, i.e., with EBCDIC:
>
> Seriously who gives a damn about EBCDIC anymore.
Daniel and Daniel,
Personally, I don't care at all, but I suspect that people using z/OS
and OS/390 care, among others. Some might even be Red Hat customers.
What's wrong with making the code portable, just in case?
Portability while certainly worthwhile isn't absolute - we only have finite
development time. While we could spend that time worrying about EBCDIC and
other platforms we're not likely to encounter any time soon, there is a
tradeoff to be made at some point where we have to get back to actually
developing features that matter to the 99.99999% of our userbase who isn't
EBCDIC. I'm not saying we should go out of our way to break other platforms,
but there's a cost/benefit tradeoff to be had.
> This just makes it totally unreadable.
This just makes *what* totally unreadable?
The giant switch statements is what I was referring to.
I proposed to do this:
for (p = str; *p; ++p) {
is_alphanum (*p)
...
for (p = str; *p; ++p) {
is_alphanum (*p)
I've no objection to this - just the giant switch statement.
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|