Although I’m a GNOME and gtkmm guy, my paid work involves Qt. It’s no secret that I have a very strong antipathy towards the Qt mania to assimilate everything into their framework, the culture of their community, and their nonchalant abuse of the C++ programming language. Often, people suggest that I’m exaggerating the faults of Qt, and that there are much worse alternatives out there. Fair enough.
But today I happened onto another one of those little inanities which so often add up to a huge stinking pile. I decided to share this particular example, as I think it may help people to understand why I’m feeling so strongly about Qt.
A while ago, I filed Qt bug #5732, together with a patch to fix the problem. In addition to the fix for the central problem reported in the bug, I also provided a second patch to correct another problem I stumbled upon while I worked on the actual bug fix. The bug was acknowledged by a Qt developer and the code got fixed. They decided to fix it their own way instead of applying my patch, which is fair enough. I may not have agreed with the way it was implemented, but the problem I reported was fixed.
Today, I again looked at the Qt OpenGL code, and happened to stumble upon the OpenGL extension checks. Apparently, the extension checking had now finally been factored out into a separate class, with a more efficient implementation to avoid the numerous temporary memory allocations of the string splitting method that was used before. Of course I was curious and had a look at the code which implements the string parsing.
Here is how it looks like:
// This class can be used to match GL extensions without doing any mallocs. The
// class assumes that the GL extension string ends with a space character,
// which it should do on all conformant platforms. Create the object and pass
// in a pointer to the extension string, then call match() on each extension
// that should be matched. The match() function takes the extension name
// *without* the terminating space character as input.
class QGLExtensionMatcher
{
public:
QGLExtensionMatcher(const char *str)
: gl_extensions(str), gl_extensions_length(qstrlen(str))
{}
bool match(const char *str) {
int str_length = qstrlen(str);
const char *extensions = gl_extensions;
int extensions_length = gl_extensions_length;
while (1) {
// the total length that needs to be matched is the str_length +
// the space character that terminates the extension name
if (extensions_length < str_length + 1)
return false;
if (qstrncmp(extensions, str, str_length) == 0 && extensions[str_length] == ' ')
return true;
int split_pos = 0;
while (split_pos < extensions_length && extensions[split_pos] != ' ')
++split_pos;
++split_pos; // added for the terminating space character
extensions += split_pos;
extensions_length -= split_pos;
}
return false;
}
private:
const char *gl_extensions;
int gl_extensions_length;
};
According to the log message the piece of code just shown went through internal code review. For comparison, here is the equivalent piece of code from my original patch, which was not considered good enough to be applied:
static bool parse_extensions_string(const char* extensions, const char* name)
{
const size_t name_length = (name) ? strlen(name) : 0;
Q_ASSERT(name_length > 0);
if (extensions) {
const char* pos = extensions;
while (const char *const space = strchr(pos, ' ')) {
const size_t length = space - pos;
if (length == name_length && memcmp(pos, name, length) == 0)
return true;
pos = space + 1;
}
return (strcmp(pos, name) == 0);
}
return false;
}
So, instead of just taking the code handed to them on a silver platter, the Qt developers decided to roll their own version. Which I wouldn’t even consider a problem if it weren’t for the fact that the code that went in is longer, less readable, and generally ugly. It’s also broken, because there is not a single word in the OpenGL specification about glGetString() always returning a list terminated by a space character, contrary to what the code comment claims about “all conformant platforms”.
March 18th, 2010 by
Daniel Elstner