One useful comment

22 Mar 2021

Author: Andrey Karpov

Most influential programmers say that code must be self-documenting. They find comments useful only when working with something uncommon. Our team shares this opinion. Recently we came across a code snippet that perfectly proves it.

We wrote out the following code while working with the article "Date processing attracts bugs or 77 defects in Qt 6".

The PVS-Studio analyzer highlighted this code snippet and issued the warning: V575 [CWE-628] The 'memcpy' function doesn't copy the whole string. Use 'strcpy / strcpy_s' function to preserve terminal null. qplaintestlogger.cpp 253. Actually, here it is:

const char *msgFiller = msg[0] ? " " : "";
QTestCharBuffer testIdentifier;
QTestPrivate::generateTestIdentifier(&testIdentifier);
QTest::qt_asprintf(&messagePrefix, "%s: %s%s%s%s\n",
                   type, testIdentifier.data(), msgFiller, msg,
                   failureLocation.data());

// In colored mode, printf above stripped our nonprintable control characters.
// Put them back.
memcpy(messagePrefix.data(), type, strlen(type));

outputMessage(messagePrefix.data());

Note the call to the memcpy function. This code itself raises two questions at once:

Something is written to buffer, the contents of which were just generated with a printf-like function. Who does that?
The terminal zero is not copied. Are you sure that's not an error? The analyzer doesn't like it.

Fortunately, the comment immediately makes it clear. Some non-printed characters must be restored.

Here's a necessary and useful text. It's a great comment explaining an unclear point in the code. It may be used as an example in how-to articles.

For comparison, take a look at another code snippet from the same file:

char buf[1024];

if (result.setByMacro) {
  qsnprintf(buf, sizeof(buf), "%s%s%s%s%s%s\n", buf1, bufTag, fill,
            buf2, buf2_, buf3);
} else {
  qsnprintf(buf, sizeof(buf), "%s%s%s%s\n", buf1, bufTag, fill, buf2);
}

memcpy(buf, bmtag, strlen(bmtag));
outputMessage(buf);

The programmer forgot to leave a similar comment here. That's why, everything's changed. A new team member who has to maintain and modify this code may be a little confused. It's not clear at all why this memcpy is used here. Moreover, it is not clear why the contents of a certain buffer buf1 were printed at the beginning of the line and why the contents of the buffer bmtag are appended at the beginning of the string. So many questions, so few answers. Don't write that way.

#Cpp