this is a new check for clang-tidy that diagnoses when it see unusual array index syntax.
there is nothing wrong about such syntax so it's not a misc check. It's just a readability check.
Differential D21134
clang-tidy: new check readability-misplaced-array-index danielmarjamaki on Jun 8 2016, 6:55 AM. Authored by
Details
this is a new check for clang-tidy that diagnoses when it see unusual array index syntax. there is nothing wrong about such syntax so it's not a misc check. It's just a readability check.
Diff Detail
Event TimelineComment Actions updated releasedocs Comment Actions The check seems reasonable, I'm surprised there's no warning in Clang that catches index[array] syntax ;) A few comments re: implementation.
Comment Actions Thanks, that's better. Still a couple of comments.
Comment Actions Is there a reason we don't want this check to be a part of the clang frontend, rather than as a clang-tidy check?
Comment Actions I think the replacement is wrong for something like: int *arr; int offs1, offs2; offs1[arr + offs2] = 42; which I think would give: int *arr; int offs1, offs2; arr + offs2[offs1] = 42; If the precedence of the "indexing" argument is less than that of the subscript operator, then you need to insert parens: int *arr; int offs1, offs2; (arr + offs2)[offs1] = 42; Comment Actions Indeed. It might be relevant to future (current ones seem to be fine) uses of tooling::fixit::createReplacement as well, so should probably be taken care of in its implementation. Maybe an additional hint from the calling code is needed (bool InsertParensIfNeeded = false).
Comment Actions I discussed this with frontend and clang-tidy people in the llvm meeting last year and we came to the conclusion it was better in clang-tidy. I don't remember the exact reasons. But I think that this code is just "weird". It could be a bug but I write the warning mostly for readability reasons.
Comment Actions I don't think so. The matcher says: hasRHS(ignoringParenImpCasts( anyOf(stringLiteral(), declRefExpr(hasType(pointsTo(qualType()))), memberExpr(hasType(pointsTo(qualType())) I think it's hard to know what the replaced code should be. In some cases it might be better with: arr[offs1 + offs2] = 42; Comment Actions as far as I see the only unsolved review comment now is about macros. if it can be handled better.
Comment Actions A few nits.
Comment Actions Looks good. Please ensure it works reasonably on LLVM code before submitting. Sorry for the delay. Feel free to ping earlier. |