Re: [Libvir] [PATCH] Enhance virBuffer code
"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?
This just makes it totally unreadable.
This just makes *what* totally unreadable?
I proposed to do this:
for (p = str; *p; ++p) {
is_alphanum (*p)
...
for (p = str; *p; ++p) {
is_alphanum (*p)
in place of the code above with all the comparisons. Or maybe I'm
misinterpreting something and you're saying the 11-operations-per-byte
version is harder to read/maintain? In that case, I would agree.
Encapsulating the test makes it _more_ readable and maintainable,
since there is far less syntax, less logic, and less duplication.
If you're saying that the definition of is_alphanum is unreadable,
I don't buy it. It's verifiably correct, and works properly even for
those unfortunate enough to be using EBCDIC. Why bother to read it when
it's so simple and automatically generated? Between an understandable
name and a comment explaining what it does, I would have thought that'd
be enough for anyone.
This isn't the first time (by a long shot) that we've had such
disagreements. It's obvious that our work philosophies are very
different, so, after giving it some thought, I believe it's best that I no
longer contribute to this project. I need to use my time more efficiently.
Good luck with libvirt.