This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] MoveChecker Pt.1: Give MisusedMovedObject checker a more consistent name.
ClosedPublic

Authored by NoQ on Nov 14 2018, 4:49 PM.

Details

Summary

I'd like to try to enable the use-after-move checker developed heroically by Peter "@szepet" Szecsi by default. While currently being a good opt-in lint check for enforcing coding style, currently it has too many false positives for an on-by-default check. The false positives seem to concentrate around objects that are fine to be used after they were moved from, even if no "state reset" method has been called on them, simply because their respective move-constructor/move-assignment leaves the object that is being moved in a well-specified state (eg., a null smart pointer or an empty set). This is not the case for STL objects, but it is the case for many custom objects.

I'll proceed to post a few patches and i'd love to hear your thoughts! The plan (of which this patch implements the first step) is as follows:

  1. Rename the checker. In our tradition, "XChecker" usually means a checker that checks if "X" is used correctly, not a checker that finds "X". Eg., MallocChecker warns on misuse of malloc()/free(), but we don't call it "MisusedMallocChecker". The proposed name is "MoveChecker", i.e. checker that checks moves.
  2. Address false positives under an on-by-default checker option. The proposed idea is to only warn by default in the following three cases:
    • If the object is a known STL object - because we know that it's left in unspecified state after move and we can memorize all state reset methods.
    • If the object is a local (or parameter) variable and hence there's no temptation to re-use the storage instead of making another local variable.
    • If the object is taken from a local (or parameter) rvalue reference, for the same reason.
  3. Make warning messages more specific based information provided by facilities introduced on step 2.
  4. Add a few missing state-reset methods.
  5. Enable the checker, as restricted on step 2, by default. Keep the aggressive variant around in case someone needs it, but i don't have any specific plans for it.

Diff Detail

Event Timeline

NoQ created this revision.Nov 14 2018, 4:49 PM
xazax.hun accepted this revision.Nov 15 2018, 4:45 AM

Looks good. Do we plan to detect problems other than use after move? Maybe it would be worth to synchronize with the tidy checker name use-after-move or is it going to cause more confusion?

This revision is now accepted and ready to land.Nov 15 2018, 4:45 AM
Szelethus accepted this revision.Nov 15 2018, 7:15 AM

LGTM, both the concept and the patch! I have read your followup patches up to part 4, I'll leave some thoughts there.

This revision was automatically updated to reflect the committed changes.