This is an archive of the discontinued LLVM Phabricator instance.

Avoid repeated replacements on loop-convert check.
ClosedPublic

Authored by angelgarcia on Sep 4 2015, 5:02 AM.

Details

Summary

The InitListExpr subtree is visited twice, this caused the check to do multiple replacements. Added a set to avoid it.

Diff Detail

Event Timeline

angelgarcia updated this revision to Diff 34024.Sep 4 2015, 5:02 AM
angelgarcia retitled this revision from to Avoid repeated replacements on loop-convert check..
angelgarcia updated this object.
angelgarcia added a reviewer: klimek.
angelgarcia added subscribers: alexfh, cfe-commits.
alexfh added inline comments.Sep 4 2015, 6:12 AM
clang-tidy/modernize/LoopConvertUtils.cpp
465

Do you need both Usages and UsageSet?

clang-tidy/modernize/LoopConvertUtils.h
211

Please use std::tie instead of std::make_tuple to avoid copies.

217

Looks like you can use std::tie here as well.

Anyway, I just found that this needs a few more changes (it still does duplicated replacementes inside some macros, and I have to find out why), so don't bother with this for now.

clang-tidy/modernize/LoopConvertUtils.cpp
465

Yes, because SmallSet doesn't provide a way to iterate over its members.

clang-tidy/modernize/LoopConvertUtils.h
211

I used std::tie here at first, but it didn't work because it tries to bind a RValue to a non-const ref.

angelgarcia updated this revision to Diff 34035.Sep 4 2015, 8:51 AM

I think this solves the duplication problem. And I think is clearer and cheaper than what I was trying to do before.

klimek added inline comments.Sep 4 2015, 9:08 AM
clang-tidy/modernize/LoopConvertUtils.cpp
465

There should be no need for a while() getExpansionLoc should always go to the most expanded location location.

clang-tidy/modernize/LoopConvertUtils.h
292–293

The result seems to be never used?

354

I don't think putting Set into the name helps. UsageLocations perhaps?

angelgarcia updated this revision to Diff 34039.Sep 4 2015, 9:15 AM
angelgarcia marked 2 inline comments as done.

Ok! I wasn't sure, and a while would be correct in both cases :)

klimek added inline comments.Sep 4 2015, 9:24 AM
clang-tidy/modernize/LoopConvertUtils.cpp
463–470

Now that I take a step back - shouldn't we go to the *spelling* location to deduplicate? The spelling location is the location that will be edited, after all.

Yes, with SpellingLoc works as well, and it should be better.

alexfh accepted this revision.Sep 4 2015, 2:06 PM
alexfh added a reviewer: alexfh.

Looks good with a comment.

clang-tidy/modernize/LoopConvertUtils.h
223

You're right. I missed that getBegin()/getEnd() return values, not const references. Anyway, it's irrelevant now.

292

The "Returns true" part is wrong now.

This revision is now accepted and ready to land.Sep 4 2015, 2:06 PM
angelgarcia updated this revision to Diff 34076.Sep 4 2015, 2:09 PM
angelgarcia edited edge metadata.

Wooops, solved.

angelgarcia closed this revision.Sep 4 2015, 2:38 PM
klimek edited edge metadata.Sep 5 2015, 1:05 AM

Note that I think we should add a regression test that will work with the spelling loc but not with the expansion loc.
Something like:
#define A \

<loop1> \
<loop2>

A