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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Many of the comments are marked done, but the code does not reflect that it actually is done, so I'm not certain what's happened there.



================
Comment at: clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp:86
+  diag(Alloc->getLocStart(),
+       "Addition operator is applied to the argument of "
+       "%0 instead of its result; surround the addition subexpression with "
----------------
Addition -> addition

(Diagnostics are never complete sentences with capitalization and punctuation, unlike comments.)


================
Comment at: clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp:87-88
+       "Addition operator is applied to the argument of "
+       "%0 instead of its result; surround the addition subexpression with "
+       "parentheses to silence this warning")
+      << StrLen->getDirectCallee()->getName() << Hint;
----------------
The downside to this new wording is that it contradicts the fixit hint that's being supplied. The user sees "surround the addition with parens" as the textual advice, but the fixit shows a different way to silence the warning.

I'm not certain of the best way to address that, however. Basically, there are two ways this could be fixed and both could come with fixits, but I don't believe we have a way to do an either/or pair of fixits.

We could add "or hoist the addition" to the diagnostic, but that may be too cryptic and the diagnostic would be quite long. @alexfh -- do you have ideas?


================
Comment at: clang-tidy/bugprone/MisplacedOperatorInStrlenInAllocCheck.cpp:64-67
+  const auto StrLenText = Lexer::getSourceText(
+      CharSourceRange::getTokenRange(StrLen->getSourceRange()),
+      *Result.SourceManager, getLangOpts());
+  const auto StrLenBegin = StrLenText.substr(0, StrLenText.find('(') + 1);
----------------
aaron.ballman wrote:
> Please don't use `auto` as the type is not spelled out in the initialization. Same elsewhere as well.
You marked this as done but I still see `auto` used improperly on this line and many others.


================
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
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > This comment is no longer accurate and should be reworded.
> Still not quite right because it's talking about subtraction.
It's still talking about subtraction, so this does not appear to be done.


================
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
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > This comment is no longer accurate and should be reworded.
> Same comment about subtraction.
This is also not done.


https://reviews.llvm.org/D39121