[PATCH] D32346: [clang-tidy] New readability check for strlen argument

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.


================
Comment at: clang-tidy/readability/StrlenArgumentCheck.cpp:23-24
+  Finder->addMatcher(
+      callExpr(anyOf(callee(functionDecl(hasName("::strlen"))),
+                     callee(functionDecl(hasName("std::strlen")))),
+               hasAnyArgument(ignoringParenImpCasts(
----------------
`anyOf(callee(functionDecl(hasName(x))), callee(functionDecl(hasName(y))))` unnecessarily duplicates `callee` and `functionDecl`. A better option is `callee(functionDecl(hasAnyName(x, y)))`.


================
Comment at: docs/clang-tidy/checks/readability-strlen-argument.rst:8
+
+In the example code below the developer probably wanted to make room for an extra char in the allocation but misplaced the addition.
+
----------------
JonasToth wrote:
> danielmarjamaki wrote:
> > JonasToth wrote:
> > > when the intend was to allocate one more char, he would need to do `strlen(s) + 1`, why is it changed to subtraction then?
> > If I change it to strlen(s) + 1 then the logic of the program is changed.
> > 
> > If I change it to subtraction then the logic is the same and the program is still wrong, but imho it is easier to see.
> it should be made clear in the docs, that the code is bad, especially if its UB. the dangers of copy & paste ;) (even its unlikely that this will be copy&pasted).
If we have good reasons to think the original code is wrong, so it seems that it's better to fix it, than to retain the behavior and just hide the bug?


Repository:
  rL LLVM

https://reviews.llvm.org/D32346