This revision was automatically updated to reflect the committed changes.
Closed by commit rL281206: [clang-tidy] readability-misplaced-array-index: add
new check that warns when… (authored by danielmarjamaki).
Changed prior to commit:
https://reviews.llvm.org/D21134?vs=69558=70994#toc
danielmarjamaki marked 4 inline comments as done.
danielmarjamaki added a comment.
https://reviews.llvm.org/D21134
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
danielmarjamaki updated this revision to Diff 69558.
danielmarjamaki marked 2 inline comments as done.
danielmarjamaki added a comment.
fix review comments
https://reviews.llvm.org/D21134
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/MisplacedArrayIndexCheck.cpp
alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.
A few nits.
Comment at: clang-tidy/readability/MisplacedArrayIndexCheck.h:19
@@ +18,3 @@
+
+/// Warn about unusual array index syntax (index[array] instead of
+///
danielmarjamaki marked 6 inline comments as done.
Comment at: clang-tidy/readability/MisplacedArrayIndexCheck.cpp:57
@@ +56,2 @@
+} // namespace tidy
+} // namespace clang
I removed hasMacroId() and use fixit::getText(). The replacements look good now.
danielmarjamaki updated this revision to Diff 69113.
danielmarjamaki added a comment.
fixed review comments
https://reviews.llvm.org/D21134
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/MisplacedArrayIndexCheck.cpp
clang-tidy/readability/MisplacedArrayIndexCheck.h
alexfh requested changes to this revision.
This revision now requires changes to proceed.
Comment at: clang-tidy/readability/MisplacedArrayIndexCheck.cpp:56
@@ +55,3 @@
+ if (hasMacroID(ArraySubscriptE) ||
+
danielmarjamaki marked an inline comment as done.
danielmarjamaki added a comment.
https://reviews.llvm.org/D21134
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
danielmarjamaki updated this revision to Diff 67338.
danielmarjamaki added a comment.
ran clang-format
https://reviews.llvm.org/D21134
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/MisplacedArrayIndexCheck.cpp
clang-tidy/readability/MisplacedArrayIndexCheck.h
danielmarjamaki updated this revision to Diff 67337.
danielmarjamaki added a comment.
More generic diagnostic. Diagnose all integerType[pointerType] expressions.
https://reviews.llvm.org/D21134
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/MisplacedArrayIndexCheck.cpp
jroelofs added a comment.
In https://reviews.llvm.org/D21134#509522, @danielmarjamaki wrote:
> In https://reviews.llvm.org/D21134#508524, @jroelofs wrote:
>
> > I think the replacement is wrong for something like:
> >
> > int *arr; int offs1, offs2;
> > offs1[arr + offs2] = 42;
> >
> >
> >
danielmarjamaki marked 2 inline comments as done.
Comment at: test/clang-tidy/readability-misplaced-array-index.cpp:31
@@ +30,3 @@
+ x[10] = 0;
+ dostuff(0[0 + ABC]);
+}
aaron.ballman wrote:
> Why is this considered to be "normal syntax" and not worth getting a
aaron.ballman added inline comments.
Comment at: test/clang-tidy/readability-misplaced-array-index.cpp:31
@@ +30,3 @@
+ x[10] = 0;
+ dostuff(0[0 + ABC]);
+}
Why is this considered to be "normal syntax" and not worth getting a diagnostic?
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 @@
+
danielmarjamaki updated this revision to Diff 67322.
danielmarjamaki added a comment.
take care of review comments
https://reviews.llvm.org/D21134
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/MisplacedArrayIndexCheck.cpp
danielmarjamaki added a comment.
In https://reviews.llvm.org/D21134#508524, @jroelofs wrote:
> 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 +
danielmarjamaki marked an inline comment as done.
danielmarjamaki added a comment.
In https://reviews.llvm.org/D21134#508511, @aaron.ballman wrote:
> 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?
I discussed this with
aaron.ballman added inline comments.
Comment at: clang-tidy/readability/MisplacedArrayIndexCheck.cpp:48
@@ +47,3 @@
+
+ auto D =
+ diag(ArraySubscriptE->getLocStart(),
alexfh wrote:
> aaron.ballman wrote:
> > Should not use `auto` here because the type is
alexfh added inline comments.
Comment at: clang-tidy/readability/MisplacedArrayIndexCheck.cpp:48
@@ +47,3 @@
+
+ auto D =
+ diag(ArraySubscriptE->getLocStart(),
aaron.ballman wrote:
> Should not use `auto` here because the type is not spelled out in the
>
alexfh added a comment.
In https://reviews.llvm.org/D21134#508524, @jroelofs wrote:
> 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;
aaron.ballman added a subscriber: aaron.ballman.
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a comment.
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 at:
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 []");
+
danielmarjamaki removed rL LLVM as the repository for this revision.
danielmarjamaki updated this revision to Diff 67145.
danielmarjamaki added a comment.
Cleanup my previous commit. Ran clang-format.
https://reviews.llvm.org/D21134
Files:
clang-tidy/readability/CMakeLists.txt
danielmarjamaki set the repository for this revision to rL LLVM.
danielmarjamaki updated this revision to Diff 67144.
danielmarjamaki added a comment.
Fixed review comments.
Repository:
rL LLVM
https://reviews.llvm.org/D21134
Files:
clang-tidy/readability/CMakeLists.txt
danielmarjamaki added inline comments.
Comment at: clang-tidy/readability/MisplacedArrayIndexCheck.cpp:43
@@ +42,3 @@
+
+static StringRef getAsString(const MatchFinder::MatchResult ,
+ const Expr *E) {
alexfh wrote:
> Looks like this
alexfh added a comment.
The check seems reasonable, I'm surprised there's no warning in Clang that
catches index[array] syntax ;)
A few comments re: implementation.
Comment at: clang-tidy/readability/MisplacedArrayIndexCheck.cpp:43
@@ +42,3 @@
+
+static StringRef
danielmarjamaki updated this revision to Diff 60804.
danielmarjamaki added a comment.
fixed typo in releasenotes
http://reviews.llvm.org/D21134
Files:
clang-tidy/readability/CMakeLists.txt
clang-tidy/readability/MisplacedArrayIndexCheck.cpp
danielmarjamaki removed rL LLVM as the repository for this revision.
danielmarjamaki updated this revision to Diff 60803.
danielmarjamaki added a comment.
updated releasedocs
deeper lookup if macro is used
only warn when the replacement can be done safely. the expressions "x[y+z]" and
y+z[x]"
Eugene.Zelenko added a subscriber: Eugene.Zelenko.
Eugene.Zelenko added a comment.
Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).
Repository:
rL LLVM
http://reviews.llvm.org/D21134
___
cfe-commits mailing list
danielmarjamaki created this revision.
danielmarjamaki added a reviewer: alexfh.
danielmarjamaki added a subscriber: cfe-commits.
danielmarjamaki set the repository for this revision to rL LLVM.
this is a new check for clang-tidy that diagnoses when it see unusual array
index syntax.
there is
30 matches
Mail list logo
