This is an archive of the discontinued LLVM Phabricator instance.

Fix several corner cases for loop-convert check.
ClosedPublic

Authored by angelgarcia on Sep 1 2015, 6:49 AM.

Diff Detail

Event Timeline

angelgarcia updated this revision to Diff 33687.Sep 1 2015, 6:49 AM
angelgarcia retitled this revision from to Fix several corner cases for loop-convert check..
angelgarcia updated this object.
angelgarcia added a reviewer: alexfh.
angelgarcia added subscribers: klimek, cfe-commits.
klimek added a comment.Sep 1 2015, 7:13 AM

Nice!

clang-tidy/modernize/LoopConvertCheck.cpp
378

(not necessarily in this CL) please rename E to Expression or similar; I'm fine with one-letter variables for small scopes, but struct scopes are basically infinite.

test/clang-tidy/modernize-loop-convert-basic.cpp
437

Use LLVM coding conventions for iterators (I, E) as above.

448

This test seems to be missing the it.insert(0) case that was removed from the "unsupported" comment, if I'm not missing something.

540–541

Isn't it important that it's a pointer to an unsigned const, not that the iterator method is const?

alexfh edited reviewers, added: klimek; removed: alexfh.Sep 1 2015, 7:35 AM
alexfh edited subscribers, added: alexfh; removed: klimek.
angelgarcia marked 2 inline comments as done.Sep 1 2015, 7:51 AM
angelgarcia added inline comments.
test/clang-tidy/modernize-loop-convert-basic.cpp
448

It was in the test 'modernize-loop-convert-negative.cpp'. I moved these ones there as well.

540–541

The important thing is that the check now adds a "const" whenever it is safe to do it. I changed the return type to a pointer just to be more consistent with what an iterator is supposed to be. It isn't important at all in this case.

angelgarcia updated this revision to Diff 33690.Sep 1 2015, 7:52 AM

Renamed Usage::E, moved some tests and other fixes.

klimek added inline comments.Sep 1 2015, 8:02 AM
test/clang-tidy/modernize-loop-convert-basic.cpp
448

Why is that case removed from the unsupported list then?

540–542

That makes sense.

klimek accepted this revision.Sep 1 2015, 8:05 AM
klimek edited edge metadata.

Learned that I need to learn to read, because the deleted comment was just duplicated.
Looks good, great first step towards making this significantly more useful :)

This revision is now accepted and ready to land.Sep 1 2015, 8:05 AM
angelgarcia closed this revision.Sep 1 2015, 8:06 AM