This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] MoveChecker Pt.7: NFC: Misc refactoring.
ClosedPublic

Authored by NoQ on Dec 6 2018, 2:57 PM.

Details

Summary

This is a no-functional-change part of the next patch.

The checker supports 5 different sorts of "use" after move: copying, moving, copy-assigning from, move-assigning from, calling any other method. De-duplicate the common checks that repeat 5 times into a single method, checkUse().

Re-structure the code a little bit.

Diff Detail

Repository
rC Clang

Event Timeline

NoQ created this revision.Dec 6 2018, 2:57 PM
Szelethus accepted this revision.Dec 7 2018, 9:49 AM

Woohoo! Thanks!

lib/StaticAnalyzer/Checkers/MoveChecker.cpp
512–513

How about MemRegion::getMostDerivedObjectRegion()? @baloghadamsoftware recently commited it,

This revision is now accepted and ready to land.Dec 7 2018, 9:49 AM
NoQ updated this revision to Diff 177306.Dec 7 2018, 1:58 PM
NoQ marked 2 inline comments as done.

Fxd.

Hi Artem,
The overall idea is good but I have some remarks inline.

lib/StaticAnalyzer/Checkers/MoveChecker.cpp
265

I think that if the function is named "checkUse()", committing State changes is not what is really expected from it. Should we rename it or change the logic somehow? For example, return true if a report was emitted and add transition in the caller?

272

This formatting is different from what clang-format does.

342

Should we move RD closer to its first use?

489
MisuseKind MK = CtorDecl->isMoveConstructor() ? MK_Move : MK_Copy;
checkUse(State, ArgRegion, RD, MK, C);

?

NoQ updated this revision to Diff 178272.Dec 14 2018, 1:27 PM
NoQ marked 4 inline comments as done.

Fxd.

lib/StaticAnalyzer/Checkers/MoveChecker.cpp
265

Good point. Renamed to modelUse() because the addTransition() logic becomes more complicated in the next patch, so i didn't want to duplicate it on all those call sites.

272

This gets overwritten in the next patch anyway. But imho this is more fancy than what clang-format does.

This revision was automatically updated to reflect the committed changes.