6
\$\begingroup\$

I came across the following piece of code in a UI application I need to maintain.

int tool_unhex( char c )
{
    return( c >= '0' && c <= '9' ? c - '0'
        : c >= 'A' && c <= 'F' ? c - 'A' + 10
        : c - 'a' + 10 );
}

void unescape2QString(const char *sOrg, QString & str)
{
/*
 * Remove URL hex escapes from s... done in place.  The basic concept for
 * this routine is borrowed from the WWW library HTUnEscape() routine.
 */
    char* s = (char*)sOrg;
    unsigned short w = 0;

    str = "";

    for ( ; *s != '\0'; ++s ) {
        if ( *s == '%' ) {
            if(*(s+1) == 'u')
            {
                s++;
                if ( *++s != '\0' ) {
                    w = (wchar_t) (tool_unhex( *s ) << 12);
                }
                if ( *++s != '\0' ) {
                    w += (wchar_t) (tool_unhex( *s ) << 8);
                }
                if ( *++s != '\0' ) {
                    w += (wchar_t) (tool_unhex( *s ) << 4);
                }
                if ( *++s != '\0' ) {
                    w += (wchar_t) tool_unhex( *s );
                }

                str += QString::fromUtf16(&w, 1);
            }
        }
        else
        {
            str += QString::fromAscii(s, 1);
        }
    }
}
  1. Isn't QString immutable? The comment says it's changing in place, but isn't each str += creating a new QString?
  2. Is this the most optimized way of doing this?

It looks like the function is trying to implement HTUnEscape() from w3.org, with UTF16 strings.

\$\endgroup\$
  • 4
    \$\begingroup\$ Well, not really optimising, but for your four if ( *++s != '\0' ) conditions, if any but the last one fails, the loop will go off the end of the string and keep going - until it hits another \0. \$\endgroup\$ – William Morris Apr 10 '13 at 2:30
7
\$\begingroup\$

Firstly, to answer your first question, QString is definitely not immutable. Directly from the documentation:

This operation is typically very fast (constant time), because QString preallocates extra space at the end of the string data so it can grow without reallocating the entire string each time.

I'm not a fan of the nested ternary statements in tool_unhex. Also, it does no error checking to make sure that whatever it is passed is actually a valid hex character. This is dangerous, as you use an unsigned short to store the shifted value of the character. If it's some character that's above F or f, then shifting and adding will potentially overflow and cause hard to track down bugs. For example, if there was a g that got in there somehow:

'g' - 'a' + 10;   // Equals 16
unsigned short w = (16 << 12) + (16 << 8) + (16 << 4) + 16;  // Overflow

I'd suggest changing it to the following:

#include <cctype>

// Converts the given (ASCII encoded) hex character to 
// an integer value 0 - 15.
// throw std::out_of_range if the given character is not
// a valid hex character.
int tool_unhex(char c)
{
    if(std::isdigit(c)) 
        return c - '0';
    else if(c >= 'A' && c <= 'F')
        return c - 'A' + 10;
    else if(c >= 'a' && c <= 'f')
        return c - 'a' + 10;
    throw std::out_of_range("Not a hex character");
}

If you're sure that the character passed in will always be in range, this should be documented somewhere. Also, it should be documented that this will ONLY work for ASCII and ASCII-compatible encodings.

Your unescape method has some confusing parts. The comments and the code don't match.

char* s = (char*)sOrg;

Why cast away the constness of sOrg? You don't modify s anyway, so just work directly on the (const) sOrg.

str = "";

If you're just going to clear the QString passed in straight away, why not just create and return it instead of taking it as a reference?

if ( *s == '%' ) {
    if(*(s+1) == 'u')
    {
        s++;
        if ( *++s != '\0' ) {
            w = (wchar_t) (tool_unhex( *s ) << 12);
        }
        if ( *++s != '\0' ) {
            w += (wchar_t) (tool_unhex( *s ) << 8);
        }
        if ( *++s != '\0' ) {
            w += (wchar_t) (tool_unhex( *s ) << 4);
        }
        if ( *++s != '\0' ) {
            w += (wchar_t) tool_unhex( *s );
        }

        str += QString::fromUtf16(&w, 1);
    }
}

This can certainly be simplified with a loop.

In the end, I came up with something like this:

QString unescape2QString(const char *sOrg)
{
    QString str;

    for ( ; *sOrg != '\0'; ++sOrg ) {
        if (*sOrg == '%' && *(sOrg+1) == 'u') {
        {
           unsigned short w = 0;
           ++sOrg;
            // Left shift value
            int i = 12;
            while(*++sOrg != '\0' && i >= 0) {
                w += (wchar_t) (tool_unhex(*sOrg) << i);
                i -= 4;
            }
            str += QString::fromUtf16(&w, 1);
        }
        else 
        {
            str += QString::fromAscii(s, 1);
        }
    }
    return str;
}  

This definitely needs some documentation as to what exactly it's doing, as it is absolutely not clear currently.

\$\endgroup\$
5
\$\begingroup\$
  • I can't quite tell what tool_unhex() is supposed to do. It looks like it converts a character (presumably a hex value) to a base-10 value (an int in this case). You may need to change the name to reflect on its primary purpose.

    The ternary is a little confusing. I almost mistook the last two statements as a redundancy since they only differ by one character. I'd recommend putting some comments stating what it's doing, or rewriting it to be a little clearer. Otherwise, there may be an STL functionality that serves the same purpose. In C++, it's best to utilize the library as much as possible.

  • Overall, you use many magic numbers (and chars). In cases where ASCII values are used as offsets (such as in tool_unhex()), you may not need to define them. In unescape2QString(), however, you may need to at least add comments to state their significance. Otherwise, give any of them constants with relevant names.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.