This is an archive of the discontinued LLVM Phabricator instance.

Avoid using rvalue references with trivially copyable types.
ClosedPublic

Authored by angelgarcia on Sep 7 2015, 6:14 AM.

Details

Summary

When the dereference operator returns a value that is trivially
copyable (like a pointer), copy it. After this change, modernize-loop-convert
check can be applied to the whole llvm source code without breaking any build
or test.

Diff Detail

Event Timeline

angelgarcia updated this revision to Diff 34155.Sep 7 2015, 6:14 AM
angelgarcia retitled this revision from to Avoid using rvalue references with trivially copyable types..
angelgarcia updated this object.
angelgarcia added a reviewer: klimek.
angelgarcia added subscribers: cfe-commits, alexfh.
alexfh accepted this revision.Sep 7 2015, 6:27 AM
alexfh added a reviewer: alexfh.

Awesome! Looks good.

This revision is now accepted and ready to land.Sep 7 2015, 6:27 AM
klimek added inline comments.Sep 7 2015, 6:42 AM
clang-tidy/modernize/LoopConvertCheck.cpp
555–556

I'd add (non-const):
... we have at least one (non-const) Usage...

659–664

Can you add a comment what FixerKind is in the else, so it's obvious why we don't want to do anything else in here?

667

Btw, upstream style is to just use implicit null checks; probably not something you wrote, but good to change while we're at it.

clang-tidy/modernize/LoopConvertCheck.h
35–49

I find it somewhat distressing that the variable order is completely different. Perhaps it's time to pull out a struct that encapsulates the specific attributes of the loop (the bools)?

angelgarcia marked 3 inline comments as done.Sep 7 2015, 8:06 AM
angelgarcia added inline comments.
clang-tidy/modernize/LoopConvertCheck.cpp
659–664

What do you mean? We're still inside the "if (FixerKind == LFK_Iterator)".

angelgarcia updated this revision to Diff 34158.Sep 7 2015, 8:06 AM
angelgarcia edited edge metadata.

Add struct with the parameters of the replacement. Also fix a potential source of crashes from a previous patch.

klimek added inline comments.Sep 7 2015, 8:24 AM
clang-tidy/modernize/LoopConvertCheck.cpp
659–664

Ah, sorry, misread. So, why don't we want to check for trivial copy-ability for all types?

clang-tidy/modernize/LoopConvertCheck.h
28

Add a comment.

angelgarcia updated this revision to Diff 34160.Sep 7 2015, 8:30 AM

Avoid relying in a single usage for determining if the type is trivially copyable.

angelgarcia added inline comments.Sep 7 2015, 8:32 AM
clang-tidy/modernize/LoopConvertCheck.cpp
659–664

Yes, but the arrays never return rvalues, and the iterator and pseudoarrays are "handled" in different parts in the code. I think I updated both of them, and I ensured that there was a test for each.

klimek added inline comments.Sep 7 2015, 8:32 AM
clang-tidy/modernize/LoopConvertCheck.cpp
555–564

a) please add a regression test for everything you find during development
b) I think the comment that we can assume that we have at least one non-const usage is not helpful any more, as we're not just accessing [0] any more.

klimek added inline comments.Sep 7 2015, 8:37 AM
clang-tidy/modernize/LoopConvertCheck.cpp
659–664

Ah, ok. I think that code is really hard to follow, and we'll want to refactor the flow (not in this patch though).

angelgarcia updated this revision to Diff 34162.Sep 7 2015, 9:01 AM

This is the test case I was worried about.

klimek accepted this revision.Sep 8 2015, 2:01 AM
klimek edited edge metadata.

LG

This revision was automatically updated to reflect the committed changes.