[PATCH] D21134: clang-tidy: new check readability-misplaced-array-index

[Alexander Kornienko via cfe-commits]

alexfh added a comment.

Thanks, that's better. Still a couple of comments.


================
Comment at: clang-tidy/readability/MisplacedArrayIndexCheck.cpp:50
@@ +49,3 @@
+      diag(ArraySubscriptE->getLocStart(),
+           "unusual array index syntax, usually the index is inside the []");
+
----------------
I'd say "confusing" instead of "unusual". This would also help avoiding the repetition ("unusual ..., usually ..."):

"confusing array index syntax; usually, the index is inside the brackets"

================
Comment at: clang-tidy/readability/MisplacedArrayIndexCheck.cpp:54
@@ +53,3 @@
+  // a fixit for.
+  if (hasMacroID(ArraySubscriptE) ||
+      !Result.SourceManager->isWrittenInSameFile(ArraySubscriptE->getLocStart(),
----------------
What exactly is the recursive `hasMacroID` function trying to prevent? Do you have a test that breaks if you just check that the start location of the expression is not a macro?

In most cases, it's enough to check that the whole range is on the same macro expansion level using `Lexer::makeFileCharRange` in order to prevent most problems with macros, while allowing fixes in safe cases, e.g. replacements in parameters of `EXPECT_*` macros from gtest.


https://reviews.llvm.org/D21134