Page MenuHomePhabricator

Make the modernize-loop-convert's const-detection smarter.
ClosedPublic

Authored by angelgarcia on Oct 30 2015, 6:27 AM.

Details

Summary

Now, it detects that several kinds of usages are can't modify the elements. Examples:
-When an usage is a call to a const member function or operator of the element.
-If the element is used as an argument to a function or constructor that takes a const-reference or a value.
-LValue to RValue conversion, if the element is a fundamental type (which allows the use of most of the builtin operators).

Diff Detail

Event Timeline

angelgarcia retitled this revision from to Make the modernize-loop-convert's const-detection smarter..
angelgarcia updated this object.
angelgarcia added a reviewer: klimek.
angelgarcia added subscribers: alexfh, cfe-commits.
klimek added inline comments.Oct 31 2015, 6:05 PM
clang-tidy/modernize/LoopConvertCheck.cpp
371–373

Reading just this function, it is unclear what the 'container' is.

393

Can't CaptureBuRef be modified?

test/clang-tidy/modernize-loop-convert-const.cpp
234

But they are aliases. Shouldn't we then go into the const-ness of the aliases?

angelgarcia updated this revision to Diff 38881.Nov 2 2015, 2:34 AM

Improve comments and add more tests.

Reading just this function, it is unclear what the 'container' is.

It should be clearer now.

Can't CaptureBuRef be modified?

I added a comment with an explanation.

But they are aliases. Shouldn't we then go into the const-ness of the

aliases?
I wanted to test that if we take a non-const reference, we recognize that
it is a non-const usage. But if we only do it once, the check handles it
differently because it moves that declaration to the loop's statement. I
added a few tests with this particular case as well (they don't harm).

In the case of const-references, we don't need to go through their usages
to know that they won't be modified. If the reference is non-const, even if
it is used in a const way, I don't think we should do anything about it.
That would only make things more complicated: first, we would need to
traverse the subtree again looking for usages of the reference; then, if we
decide that it is not modified and we take a const-reference of the
container's element, we should also change the declaration of the reference
to make it const.

klimek added inline comments.Nov 2 2015, 8:55 AM
clang-tidy/modernize/LoopConvertCheck.cpp
371–374

Not sure that is clearer. The comment should rather say what "E" is regarding the container.

angelgarcia updated this revision to Diff 38930.Nov 2 2015, 9:01 AM

OK, is it better now?

klimek accepted this revision.Nov 2 2015, 9:03 AM
klimek edited edge metadata.

LG. Much better, thanks!

This revision is now accepted and ready to land.Nov 2 2015, 9:03 AM
angelgarcia closed this revision.Nov 2 2015, 9:05 AM