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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D39121#910735, @baloghadamsoftware wrote:

> In https://reviews.llvm.org/D39121#910715, @aaron.ballman wrote:
>
> > The diagnostic tells the user that you surround the arg to strlen with parens to silence the diagnostic, but the fixit doesn't do that -- it moves the addition to the result. That's confusing behavior. However, if you add another fixit to surround the arg with parens (and leave in the existing one), then there are conflicting fixits (you don't want to apply them both) which would also be confusing.
>
>
> Then, I think we should rephrase the diagnostic message again. I am confident that in at least 99.9% of the cases adding 1 to the argument is a mistake. So the primary suggestion should be to move the addition to the result instead. The suggestion to put the argument in extra parentheses should be secondary, maybe also put in parentheses.


Agreed -- we want to keep the fixit with moving the +1 out to the result. However, I'm still worried that there'd be no way for the user to know how to silence the warning if the code is already correct -- I don't want users to disable a useful check because they don't have guidance on how to silence the diagnostic.

Do you have concrete numbers of how many diagnostics are triggered on large code bases, and the false positive rate?


https://reviews.llvm.org/D39121