This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] fix modernize-loop-convert to retain needed array-like operator[]
ClosedPublic

Authored by poelmanc on Jan 31 2021, 10:24 PM.

Details

Summary

modernize-loop-convert handles array-like objects like vectors fairly well, but strips slightly too much information from the iteration expression by converting:

Vector<Vector<int>> X;
for (int J = 0; J < X[5].size(); ++J)
  copyArg(X[5][J]);

to

Vector<Vector<int>> X;
for (int J : X) // should be for (int J : X[5]) 
  copyArg(J);

The [5] is a call to operator[] and gets stripped by LoopConvertCheck::getContainerString. This patch fixes that and adds several test cases.

Diff Detail

Event Timeline

poelmanc created this revision.Jan 31 2021, 10:24 PM
poelmanc requested review of this revision.Jan 31 2021, 10:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2021, 10:24 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
njames93 accepted this revision.Feb 1 2021, 5:12 AM
njames93 edited reviewers, added: aaron.ballman, alexfh, njames93; removed: sammccall, hokein.
njames93 added a subscriber: njames93.

LGTM, Can you fix the formatting as well.

clang-tools-extra/test/clang-tidy/checkers/modernize-loop-convert-multidimensional.cpp
50

Not essential, but how would this handle iterating the lowest dimension.

for (int J = 0; J < Y[3][4].size(); ++J)

Though I may have a feeling the check won't even try to convert that. If so disregard this comment.

This revision is now accepted and ready to land.Feb 1 2021, 5:12 AM
poelmanc updated this revision to Diff 320704.Feb 2 2021, 12:34 AM
poelmanc marked an inline comment as done.

Fix formatting, add suggested test case (which works.)

@njames93 Thanks for the review and for accepting this revision. I lack llvm-project commit access so if it's good to go I would greatly appreciate it if you or someone could push this whenever you have have a chance. Thanks!

@njames93 Thanks for the review and for accepting this revision. I lack llvm-project commit access so if it's good to go I would greatly appreciate it if you or someone could push this whenever you have have a chance. Thanks!

Should this be committed using poelmanc <cpllvm@stellarscience.com>?

Should this be committed using poelmanc <cpllvm@stellarscience.com>?

That or Conrad Poelman <cpllvm@stellarscience.com> would be great, thanks.

Should this be committed using poelmanc <cpllvm@stellarscience.com>?

That or Conrad Poelman <cpllvm@stellarscience.com> would be great, thanks.

I committed using the email provided but its not attributed you on github. It's attributed you here though. is that email not linked to your github account?

Should this be committed using poelmanc <cpllvm@stellarscience.com>?

That or Conrad Poelman <cpllvm@stellarscience.com> would be great, thanks.

I committed using the email provided but its not attributed you on github. It's attributed you here though. is that email not linked to your github account?

Thanks for the commit, and thanks for the heads-up! I've now added the address in my github account.

Thanks for the commit, and thanks for the heads-up! I've now added the address in my github account.

Its updated now and the commit is linked to your account.