This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] MoveChecker Pt.6: Suppress the warning for the few move-safe STL classes.
ClosedPublic

Authored by NoQ on Dec 4 2018, 5:24 PM.

Details

Summary

List of such classes shamelessly copied from https://wiki.sei.cmu.edu/confluence/x/O3s-BQ - thanks @aaron.ballman!

Warnings for locals are not suppressed.

Had to pass the checker into the visitor and make some methods non-static because globals with constructors are discouraged.

Also converted use-after-move tests to use the system header simulator so that not to grow STL code duplication.

Diff Detail

Event Timeline

NoQ created this revision.Dec 4 2018, 5:24 PM
Szelethus marked an inline comment as done.Dec 5 2018, 10:13 AM

This looks great!

Had to pass the checker into the visitor and make some methods non-static because globals with constructors are discouraged.

How about making the set constexpr?

lib/StaticAnalyzer/Checkers/MoveChecker.cpp
123

Why not const reference?

test/Analysis/diagnostics/explicit-suppression.cpp
22

Can't we just change this to // expected-warning{{Called C++ object pointer is null}}? This file is so tiny, I think it wouldn't cause much confusion, and reduces unnecessary maintenance work.

test/Analysis/use-after-move.cpp
16

This is cool, thanks!

NoQ updated this revision to Diff 176856.Dec 5 2018, 10:43 AM
NoQ marked an inline comment as done.

Convert the pointer to a reference.

Had to pass the checker into the visitor and make some methods non-static because globals with constructors are discouraged.

How about making the set constexpr?

Mmm, i don't think it works this way, even std::set can't be made constexpr yet as of C++17, let alone an LLVM custom collection. It should be possible to make a constexpr array, but we'll lose the presumably-faster lookup this way, so not really worth it; the cost is minimal anyway.

test/Analysis/diagnostics/explicit-suppression.cpp
22

I don't think it'll work. The warning is not on this line, it is in system-header-simulator-cxx.h, so we need to specify it somehow, and it'll appear only in this test, not in other tests that include that header, so we can't put it directly into the header.

Szelethus accepted this revision.Dec 5 2018, 10:56 AM

AFAIK constexpr arrays can be std::sort-ed, but it probably isn't worth the effort, I tried it myself when I was working with non-checker configs, and it's a big hassle for ultimately very little gain.

test/Analysis/diagnostics/explicit-suppression.cpp
22

Ah, okay.

This revision is now accepted and ready to land.Dec 5 2018, 10:56 AM
Szelethus added inline comments.Dec 5 2018, 12:44 PM
test/Analysis/diagnostics/explicit-suppression.cpp
22

Buuuuut since this is the only warning in the file, we could get away with it of we use FileCheck! But I leave it up to you.

I had a lot of bots breaking on me because I forgot git add on this file once, so I might end up fixing it myself.

NoQ updated this revision to Diff 177048.Dec 6 2018, 2:16 PM
NoQ marked an inline comment as done.

Add one more important case: std::optional. When moved, it moves the object within it into the new optional, if any. So the object is left in unspecified state, but the state of the optional itself is well-specified.

test/Analysis/diagnostics/explicit-suppression.cpp
22

Mmm, maybe. This test is tiny enough.

a_sidorin accepted this revision.Dec 8 2018, 11:49 PM

I think the change is fine, just a minor stylish remark.

test/Analysis/Inputs/system-header-simulator-cxx.h
782

Nit: our coding rules require a space before template lbrace.

This revision was automatically updated to reflect the committed changes.
NoQ marked 2 inline comments as done.Dec 14 2018, 12:56 PM
NoQ added inline comments.
test/Analysis/Inputs/system-header-simulator-cxx.h
782

Fxd.

RKSimon added a subscriber: RKSimon.Aug 7 2020, 3:57 AM
RKSimon added inline comments.
lib/StaticAnalyzer/Checkers/MoveChecker.cpp
80

@NoQ https://bugs.llvm.org/show_bug.cgi?id=47030 is reporting that this contains a missing comma - please can you take a look?