[PATCH] D39121: [clang-tidy] Misplaced Operator in Strlen in Alloc

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I'm still a bit concerned about how to silence this diagnostic if the code is actually correct. Would it make sense to diagnose `malloc(strlen(s + 1))` but silence the diagnostic if the argument to `strlen()` is explicitly parenthesized? That means a user could silence the diagnostic by writing `malloc(strlen((s + 1)))`.



================
Comment at: clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp:23
+  const auto BadUse =
+      callExpr(callee(functionDecl(hasName("strlen"))),
+               hasAnyArgument(ignoringParenImpCasts(
----------------
This should be checking for `::strlen` instead.

What about other `strlen`-like functions (`strnlen` and `strnlen_s` come to mind)?
What about wide-character strings using `wcslen`? (This might be another good check because those should be `wcslen(str) * sizeof(wchar_t)` instead of just `wcslen(str)`.)


================
Comment at: clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp:37
+  Finder->addMatcher(callExpr(callee(functionDecl(
+                                  anyOf(hasName("malloc"), hasName("alloca")))),
+                              hasArgument(0, BadArg))
----------------
`::malloc` (etc), here and below.

The reason is because a pathological case might have these functions inside of a namespace, so you want to ensure this only checks against the global namespace.


================
Comment at: clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp:54
+
+  const std::string LHSText = Lexer::getSourceText(
+      CharSourceRange::getTokenRange(BinOp->getLHS()->getSourceRange()),
----------------
Assign into a `StringRef` to avoid a needless copy operation (same below).


================
Comment at: clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp:63
+      StrLen->getSourceRange(),
+      "strlen(" + LHSText + ")" +
+          ((BinOp->getOpcode() == BO_Add) ? " + " : " - ") + RHSText);
----------------
This should use a `Twine` to avoid needless allocations and copies.


================
Comment at: clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp:64
+      "strlen(" + LHSText + ")" +
+          ((BinOp->getOpcode() == BO_Add) ? " + " : " - ") + RHSText);
+
----------------
`BinOp->getOpcode()` will always be `BO_Add`, correct? That's the only binary operator you're matching against.


================
Comment at: clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp:66
+
+  diag(Alloc->getLocStart(), "Binary operator %0 %1 is inside strlen")
+      << ((BinOp->getOpcode() == BO_Add) ? "+" : "-") << BinOp->getRHS()
----------------
I'm not keen on this wording as it doesn't tell the user what's wrong with their code; further, it doesn't tell the user how to silence the warning if the code is correct. Perhaps:

"addition operator is applied to the argument to `strlen` instead of the result; surround the addition subexpression with parentheses to silence this warning"


================
Comment at: clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp:67
+  diag(Alloc->getLocStart(), "Binary operator %0 %1 is inside strlen")
+      << ((BinOp->getOpcode() == BO_Add) ? "+" : "-") << BinOp->getRHS()
+      << Hint;
----------------
`+` is the only binop you match.


================
Comment at: docs/ReleaseNotes.rst:63
+
+  Finds cases a value is added to or subtracted from the string in the parameter
+  of ``strlen()`` method instead of to the result and use its return value as an
----------------
This comment is no longer accurate and should be reworded.


================
Comment at: docs/clang-tidy/checks/bugprone-misplaced-operator-in-strlen-in-alloc.rst:6
+
+Finds cases a value is added to or subtracted from the string in the parameter
+of ``strlen()`` method instead of to the result and use its return value as an
----------------
This comment is no longer accurate and should be reworded.


================
Comment at: test/clang-tidy/bugprone-misplaced-operator-in-strlen-in-alloc.cpp:36
+void intentional1(char *name) {
+  char *new_name = (char*) malloc(strlen(name + 1) + 1);
+}
----------------
This should have some comments that explain why it's valid and it should be clearly spelled out in the docs.


================
Comment at: test/clang-tidy/bugprone-misplaced-operator-in-strlen-in-alloc.cpp:39
+
+
+void intentional2(char *name) {
----------------
Spurious newline.


================
Comment at: test/clang-tidy/bugprone-misplaced-operator-in-strlen-in-alloc.cpp:40
+
+void intentional2(char *name) {
+  char *new_name = (char*) malloc(strlen(name + 2));
----------------
This should have some comments that explain why it's valid and it should be clearly spelled out in the docs.


https://reviews.llvm.org/D39121