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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
