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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
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.
+
----------------
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).


================
Comment at: docs/clang-tidy/checks/readability-strlen-argument.rst:20
+    char *p = new char[(strlen(s) - 1)]
+    strcpy(p, s);
+
----------------
danielmarjamaki wrote:
> JonasToth wrote:
> > isnt that an overflow?
> > an example:
> > `strlen(s) == 10` -> `p` will be 9 characters long, since its substracted with `1`.
> > 
> > the copy operation will then copy the content of `s` into `p`, therefore copying 10 characters into a buffer of length 9.
> > 
> > as i understand it `strcpy(p, s + 1)` would be correct with the sizes.
> yes it is overflow. My intention was to show that strlen(s+1) syntax is dangerous.
ok. please state that the overflow in a comment, its better to make that explicit.


Repository:
  rL LLVM

https://reviews.llvm.org/D32346