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

[Daniel Marjamäki via cfe-commits]

danielmarjamaki marked 4 inline comments as done.
danielmarjamaki added a comment.

as far as I see the only unsolved review comment now is about macros. if it can be handled better.


================
Comment at: clang-tidy/readability/MisplacedArrayIndexCheck.cpp:28-29
@@ +27,4 @@
+              anyOf(stringLiteral(), declRefExpr(hasType(pointerType())),
+                    memberExpr(hasType(pointerType()))))))
+          .bind("expr"),
+      this);
----------------
Yes thanks!

================
Comment at: clang-tidy/readability/MisplacedArrayIndexCheck.cpp:53
@@ +52,3 @@
+  if (hasMacroID(ArraySubscriptE) ||
+      !Result.SourceManager->isWrittenInSameFile(ArraySubscriptE->getLocStart(),
+                                                 ArraySubscriptE->getLocEnd()))
----------------
you can do lots of weird things with macros. so I wanted to just diagnose and then have no fixits that would be wrong etc. I removed the comment since I think the code is pretty clear.

================
Comment at: clang-tidy/readability/MisplacedArrayIndexCheck.cpp:55
@@ +54,3 @@
+                                                 ArraySubscriptE->getLocEnd()))
+    return;
+
----------------
> 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?

I am worried about macros anywhere. I did not want to apply fixes for this code:  0[ABC]  but maybe using the Lexer would make that safe.

> 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.

I was very unsuccessful with that. I must do something wrong...
```
  CharSourceRange LRange = Lexer::makeFileCharRange(
      CharSourceRange::getCharRange(
          ArraySubscriptE->getLHS()->getSourceRange()),
      Result.Context->getSourceManager(), Result.Context->getLangOpts());

  CharSourceRange RRange = Lexer::makeFileCharRange(
      CharSourceRange::getCharRange(
          ArraySubscriptE->getLHS()->getSourceRange()),
      Result.Context->getSourceManager(), Result.Context->getLangOpts());

  StringRef LText = Lexer::getSourceText(LRange, *Result.SourceManager,
                                        Result.Context->getLangOpts());

  StringRef RText = Lexer::getSourceText(RRange, *Result.SourceManager,
                                        Result.Context->getLangOpts());

  Diag << FixItHint::CreateReplacement(ArraySubscriptE->getLHS()->getSourceRange(),
                                       RText);
  Diag << FixItHint::CreateReplacement(ArraySubscriptE->getRHS()->getSourceRange(),
                                    LText);
```
No fixits worked with that code, with or without macros.

Do you see what I did wrong?


https://reviews.llvm.org/D21134