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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
