One Useful Comment

Andrey Karpov
Articles: 565

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.

0815_good_comment/image1.png

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.


You can discuss this article with other readers on habr.com


Use PVS-Studio to search for bugs in C, C++, C# and Java

We offer you to check your project code with PVS-Studio. Just one bug found in the project will show you the benefits of the static code analysis methodology better than a dozen of the articles.

goto PVS-Studio;

Andrey Karpov
Articles: 565


Bugs Found

Checked Projects
427
Collected Errors
14 526
This website uses cookies and other technology to provide you a more personalized experience. By continuing the view of our web-pages you accept the terms of using these files. If you don't want your personal data to be processed, please, leave this site. Learn More →
Accept