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

[Simon Pilgrim via Phabricator via cfe-commits]

RKSimon added inline comments.
Herald added subscribers: steakhal, ASDenysPetrov, martong, Charusso.



Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:81
+  "optional",
+  "packaged_task"
+  "promise",

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


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55307/new/

https://reviews.llvm.org/D55307

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC349191: [analyzer] MoveChecker Pt.6: Suppress the warning 
for the move-safe STL classes. (authored by dergachev, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D55307?vs=177048=178266#toc

Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55307/new/

https://reviews.llvm.org/D55307

Files:
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  test/Analysis/Inputs/system-header-simulator-cxx.h
  test/Analysis/diagnostics/explicit-suppression.cpp
  test/Analysis/use-after-move.cpp

Index: test/Analysis/diagnostics/explicit-suppression.cpp
===
--- test/Analysis/diagnostics/explicit-suppression.cpp
+++ test/Analysis/diagnostics/explicit-suppression.cpp
@@ -19,6 +19,6 @@
 void testCopyNull(C *I, C *E) {
   std::copy(I, E, (C *)0);
 #ifndef SUPPRESSED
-  // expected-warning@../Inputs/system-header-simulator-cxx.h:670 {{Called C++ object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:677 {{Called C++ object pointer is null}}
 #endif
 }
Index: test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- test/Analysis/Inputs/system-header-simulator-cxx.h
+++ test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -237,6 +237,13 @@
 return static_cast(a);
   }
 
+  template 
+  void swap(T , T ) {
+T c(std::move(a));
+a = std::move(b);
+b = std::move(c);
+  }
+
   template
   class vector {
 typedef T value_type;
@@ -770,6 +777,22 @@
 
 }
 
+#if __cplusplus >= 201103L
+namespace std {
+  template  // TODO: Implement the stub for deleter.
+  class unique_ptr {
+  public:
+unique_ptr(const unique_ptr &) = delete;
+unique_ptr(unique_ptr &&);
+
+T *get() const;
+
+typename std::add_lvalue_reference::type operator*() const;
+T *operator->() const;
+  };
+}
+#endif
+
 #ifdef TEST_INLINABLE_ALLOCATORS
 namespace std {
   void *malloc(size_t);
Index: test/Analysis/use-after-move.cpp
===
--- test/Analysis/use-after-move.cpp
+++ test/Analysis/use-after-move.cpp
@@ -13,47 +13,7 @@
 // RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1\
 // RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE
 
-namespace std {
-
-typedef __typeof(sizeof(int)) size_t;
-
-template 
-struct remove_reference;
-
-template 
-struct remove_reference { typedef _Tp type; };
-
-template 
-struct remove_reference<_Tp &> { typedef _Tp type; };
-
-template 
-struct remove_reference<_Tp &&> { typedef _Tp type; };
-
-template 
-typename remove_reference<_Tp>::type &(_Tp &&__t) {
-  return static_cast::type &&>(__t);
-}
-
-template 
-_Tp &(typename remove_reference<_Tp>::type &__t) noexcept {
-  return static_cast<_Tp &&>(__t);
-}
-
-template 
-void swap(T , T ) {
-  T c(std::move(a));
-  a = std::move(b);
-  b = std::move(c);
-}
-
-template 
-class vector {
-public:
-  vector();
-  void push_back(const T );
-};
-
-} // namespace std
+#include "Inputs/system-header-simulator-cxx.h"
 
 class B {
 public:
@@ -832,13 +792,26 @@
 
 class HasSTLField {
   std::vector V;
-  void foo() {
+  void testVector() {
 // Warn even in non-aggressive mode when it comes to STL, because
 // in STL the object is left in "valid but unspecified state" after move.
 std::vector W = std::move(V); // expected-note{{Object 'V' of type 'std::vector' is left in a valid but unspecified state after move}}
 V.push_back(123); // expected-warning{{Method called on moved-from object 'V'}}
   // expected-note@-1{{Method called on moved-from object 'V'}}
   }
+
+  std::unique_ptr P;
+  void testUniquePtr() {
+// unique_ptr remains in a well-defined state after move.
+std::unique_ptr Q = std::move(P);
+P.get();
+#ifdef AGGRESSIVE
+// expected-warning@-2{{Method called on moved-from object 'P'}}
+// expected-note@-4{{Object 'P' is moved}}
+// expected-note@-4{{Method called on moved-from object 'P'}}
+#endif
+*P += 1; // FIXME: Should warn that the pointer is null.
+  }
 };
 
 void localRValueMove(A &) {
@@ -846,3 +819,11 @@
   a.foo(); // expected-warning{{Method called on moved-from object 'a'}}
// expected-note@-1{{Method called on moved-from object 'a'}}
 }
+
+void localUniquePtr(std::unique_ptr P) {
+  // Even though unique_ptr is safe to use after move,
+  // reusing a local variable this way usually indicates a bug.
+  std::unique_ptr Q = std::move(P); // expected-note{{Object 'P' is moved}}
+  P.get(); // expected-warning{{Method called on moved-from object 'P'}}
+   // expected-note@-1{{Method called on moved-from object 'P'}}
+}
Index: lib/StaticAnalyzer/Checkers/MoveChecker.cpp
===
--- 

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ marked 2 inline comments as done.
NoQ added inline comments.



Comment at: test/Analysis/Inputs/system-header-simulator-cxx.h:782
+namespace std {
+  template // TODO: Implement the stub for deleter.
+  class unique_ptr {

a_sidorin wrote:
> Nit: our coding rules require a space before template lbrace.
Fxd.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55307/new/

https://reviews.llvm.org/D55307



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aleksei Sidorin via Phabricator via cfe-commits]

a_sidorin accepted this revision.
a_sidorin added a comment.

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




Comment at: test/Analysis/Inputs/system-header-simulator-cxx.h:782
+namespace std {
+  template // TODO: Implement the stub for deleter.
+  class unique_ptr {

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


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55307/new/

https://reviews.llvm.org/D55307



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 177048.
NoQ marked an inline comment as done.
NoQ added a comment.

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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55307/new/

https://reviews.llvm.org/D55307

Files:
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  test/Analysis/Inputs/system-header-simulator-cxx.h
  test/Analysis/diagnostics/explicit-suppression.cpp
  test/Analysis/use-after-move.cpp

Index: test/Analysis/use-after-move.cpp
===
--- test/Analysis/use-after-move.cpp
+++ test/Analysis/use-after-move.cpp
@@ -13,47 +13,7 @@
 // RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1\
 // RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE
 
-namespace std {
-
-typedef __typeof(sizeof(int)) size_t;
-
-template 
-struct remove_reference;
-
-template 
-struct remove_reference { typedef _Tp type; };
-
-template 
-struct remove_reference<_Tp &> { typedef _Tp type; };
-
-template 
-struct remove_reference<_Tp &&> { typedef _Tp type; };
-
-template 
-typename remove_reference<_Tp>::type &(_Tp &&__t) {
-  return static_cast::type &&>(__t);
-}
-
-template 
-_Tp &(typename remove_reference<_Tp>::type &__t) noexcept {
-  return static_cast<_Tp &&>(__t);
-}
-
-template 
-void swap(T , T ) {
-  T c(std::move(a));
-  a = std::move(b);
-  b = std::move(c);
-}
-
-template 
-class vector {
-public:
-  vector();
-  void push_back(const T );
-};
-
-} // namespace std
+#include "Inputs/system-header-simulator-cxx.h"
 
 class B {
 public:
@@ -832,13 +792,26 @@
 
 class HasSTLField {
   std::vector V;
-  void foo() {
+  void testVector() {
 // Warn even in non-aggressive mode when it comes to STL, because
 // in STL the object is left in "valid but unspecified state" after move.
 std::vector W = std::move(V); // expected-note{{Object 'V' of type 'std::vector' is left in a valid but unspecified state after move}}
 V.push_back(123); // expected-warning{{Method called on moved-from object 'V'}}
   // expected-note@-1{{Method called on moved-from object 'V'}}
   }
+
+  std::unique_ptr P;
+  void testUniquePtr() {
+// unique_ptr remains in a well-defined state after move.
+std::unique_ptr Q = std::move(P);
+P.get();
+#ifdef AGGRESSIVE
+// expected-warning@-2{{Method called on moved-from object 'P'}}
+// expected-note@-4{{Object 'P' is moved}}
+// expected-note@-4{{Method called on moved-from object 'P'}}
+#endif
+*P += 1; // FIXME: Should warn that the pointer is null.
+  }
 };
 
 void localRValueMove(A &) {
@@ -846,3 +819,11 @@
   a.foo(); // expected-warning{{Method called on moved-from object 'a'}}
// expected-note@-1{{Method called on moved-from object 'a'}}
 }
+
+void localUniquePtr(std::unique_ptr P) {
+  // Even though unique_ptr is safe to use after move,
+  // reusing a local variable this way usually indicates a bug.
+  std::unique_ptr Q = std::move(P); // expected-note{{Object 'P' is moved}}
+  P.get(); // expected-warning{{Method called on moved-from object 'P'}}
+   // expected-note@-1{{Method called on moved-from object 'P'}}
+}
Index: test/Analysis/diagnostics/explicit-suppression.cpp
===
--- test/Analysis/diagnostics/explicit-suppression.cpp
+++ test/Analysis/diagnostics/explicit-suppression.cpp
@@ -19,6 +19,6 @@
 void testCopyNull(C *I, C *E) {
   std::copy(I, E, (C *)0);
 #ifndef SUPPRESSED
-  // expected-warning@../Inputs/system-header-simulator-cxx.h:670 {{Called C++ object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:677 {{Called C++ object pointer is null}}
 #endif
 }
Index: test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- test/Analysis/Inputs/system-header-simulator-cxx.h
+++ test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -237,6 +237,13 @@
 return static_cast(a);
   }
 
+  template 
+  void swap(T , T ) {
+T c(std::move(a));
+a = std::move(b);
+b = std::move(c);
+  }
+
   template
   class vector {
 typedef T value_type;
@@ -770,6 +777,22 @@
 
 }
 
+#if __cplusplus >= 201103L
+namespace std {
+  template // TODO: Implement the stub for deleter.
+  class unique_ptr {
+  public:
+unique_ptr(const unique_ptr &) = delete;
+unique_ptr(unique_ptr &&);
+
+T *get() const;
+
+typename std::add_lvalue_reference::type operator*() const;
+T *operator->() const;
+  };
+}
+#endif
+
 #ifdef TEST_INLINABLE_ALLOCATORS
 namespace std {
   void *malloc(size_t);
Index: lib/StaticAnalyzer/Checkers/MoveChecker.cpp
===
--- 

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: test/Analysis/diagnostics/explicit-suppression.cpp:22
 #ifndef SUPPRESSED
-  // expected-warning@../Inputs/system-header-simulator-cxx.h:670 {{Called C++ 
object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:677 {{Called C++ 
object pointer is null}}
 #endif

Szelethus wrote:
> Szelethus wrote:
> > NoQ wrote:
> > > Szelethus wrote:
> > > > 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.
> > > 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.
> > Ah, okay.
> But 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.
Mmm, maybe. This test is tiny enough.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55307/new/

https://reviews.llvm.org/D55307



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.



Comment at: test/Analysis/diagnostics/explicit-suppression.cpp:22
 #ifndef SUPPRESSED
-  // expected-warning@../Inputs/system-header-simulator-cxx.h:670 {{Called C++ 
object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:677 {{Called C++ 
object pointer is null}}
 #endif

Szelethus wrote:
> NoQ wrote:
> > Szelethus wrote:
> > > 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.
> > 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.
> Ah, okay.
But 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55307/new/

https://reviews.llvm.org/D55307



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus accepted this revision.
Szelethus added a comment.
This revision is now accepted and ready to land.

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.




Comment at: test/Analysis/diagnostics/explicit-suppression.cpp:22
 #ifndef SUPPRESSED
-  // expected-warning@../Inputs/system-header-simulator-cxx.h:670 {{Called C++ 
object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:677 {{Called C++ 
object pointer is null}}
 #endif

NoQ wrote:
> Szelethus wrote:
> > 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.
> 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.
Ah, okay.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55307/new/

https://reviews.llvm.org/D55307



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 176856.
NoQ marked an inline comment as done.
NoQ added a comment.

Convert the pointer to a reference.

In D55307#1320434 , @Szelethus wrote:

> > 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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55307/new/

https://reviews.llvm.org/D55307

Files:
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  test/Analysis/Inputs/system-header-simulator-cxx.h
  test/Analysis/diagnostics/explicit-suppression.cpp
  test/Analysis/use-after-move.cpp

Index: test/Analysis/use-after-move.cpp
===
--- test/Analysis/use-after-move.cpp
+++ test/Analysis/use-after-move.cpp
@@ -13,47 +13,7 @@
 // RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1\
 // RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE
 
-namespace std {
-
-typedef __typeof(sizeof(int)) size_t;
-
-template 
-struct remove_reference;
-
-template 
-struct remove_reference { typedef _Tp type; };
-
-template 
-struct remove_reference<_Tp &> { typedef _Tp type; };
-
-template 
-struct remove_reference<_Tp &&> { typedef _Tp type; };
-
-template 
-typename remove_reference<_Tp>::type &(_Tp &&__t) {
-  return static_cast::type &&>(__t);
-}
-
-template 
-_Tp &(typename remove_reference<_Tp>::type &__t) noexcept {
-  return static_cast<_Tp &&>(__t);
-}
-
-template 
-void swap(T , T ) {
-  T c(std::move(a));
-  a = std::move(b);
-  b = std::move(c);
-}
-
-template 
-class vector {
-public:
-  vector();
-  void push_back(const T );
-};
-
-} // namespace std
+#include "Inputs/system-header-simulator-cxx.h"
 
 class B {
 public:
@@ -832,13 +792,26 @@
 
 class HasSTLField {
   std::vector V;
-  void foo() {
+  void testVector() {
 // Warn even in non-aggressive mode when it comes to STL, because
 // in STL the object is left in "valid but unspecified state" after move.
 std::vector W = std::move(V); // expected-note{{Object 'V' of type 'std::vector' is left in a valid but unspecified state after move}}
 V.push_back(123); // expected-warning{{Method called on moved-from object 'V'}}
   // expected-note@-1{{Method called on moved-from object 'V'}}
   }
+
+  std::unique_ptr P;
+  void testUniquePtr() {
+// unique_ptr remains in a well-defined state after move.
+std::unique_ptr Q = std::move(P);
+P.get();
+#ifdef AGGRESSIVE
+// expected-warning@-2{{Method called on moved-from object 'P'}}
+// expected-note@-4{{Object 'P' is moved}}
+// expected-note@-4{{Method called on moved-from object 'P'}}
+#endif
+*P += 1; // FIXME: Should warn that the pointer is null.
+  }
 };
 
 void localRValueMove(A &) {
@@ -846,3 +819,11 @@
   a.foo(); // expected-warning{{Method called on moved-from object 'a'}}
// expected-note@-1{{Method called on moved-from object 'a'}}
 }
+
+void localUniquePtr(std::unique_ptr P) {
+  // Even though unique_ptr is safe to use after move,
+  // reusing a local variable this way usually indicates a bug.
+  std::unique_ptr Q = std::move(P); // expected-note{{Object 'P' is moved}}
+  P.get(); // expected-warning{{Method called on moved-from object 'P'}}
+   // expected-note@-1{{Method called on moved-from object 'P'}}
+}
Index: test/Analysis/diagnostics/explicit-suppression.cpp
===
--- test/Analysis/diagnostics/explicit-suppression.cpp
+++ test/Analysis/diagnostics/explicit-suppression.cpp
@@ -19,6 +19,6 @@
 void testCopyNull(C *I, C *E) {
   std::copy(I, E, (C *)0);
 #ifndef SUPPRESSED
-  // expected-warning@../Inputs/system-header-simulator-cxx.h:670 {{Called C++ object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:677 {{Called C++ object pointer is null}}
 #endif
 }
Index: test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- test/Analysis/Inputs/system-header-simulator-cxx.h
+++ test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -237,6 +237,13 @@
 return static_cast(a);
   }
 
+  template 
+  void swap(T , T ) {
+T c(std::move(a));
+a = std::move(b);
+b = std::move(c);
+  }
+
   template
   class vector {
 typedef T value_type;
@@ -770,6 +777,22 @@
 
 }
 
+#if __cplusplus >= 201103L
+namespace std {
+  template // TODO: Implement the stub for deleter.
+  class unique_ptr {
+  public:
+unique_ptr(const unique_ptr &) = delete;
+unique_ptr(unique_ptr &&);
+
+T 

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: test/Analysis/diagnostics/explicit-suppression.cpp:22
 #ifndef SUPPRESSED
-  // expected-warning@../Inputs/system-header-simulator-cxx.h:670 {{Called C++ 
object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:677 {{Called C++ 
object pointer is null}}
 #endif

Szelethus wrote:
> 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.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55307/new/

https://reviews.llvm.org/D55307



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus marked an inline comment as done.
Szelethus added a comment.

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`?




Comment at: lib/StaticAnalyzer/Checkers/MoveChecker.cpp:122
   private:
+const MoveChecker *Chk;
 // The tracked region.

Why not const reference?



Comment at: test/Analysis/diagnostics/explicit-suppression.cpp:22
 #ifndef SUPPRESSED
-  // expected-warning@../Inputs/system-header-simulator-cxx.h:670 {{Called C++ 
object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:677 {{Called C++ 
object pointer is null}}
 #endif

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.



Comment at: test/Analysis/use-after-move.cpp:16
 
-namespace std {
-
-typedef __typeof(sizeof(int)) size_t;
-
-template 
-struct remove_reference;
-
-template 
-struct remove_reference { typedef _Tp type; };
-
-template 
-struct remove_reference<_Tp &> { typedef _Tp type; };
-
-template 
-struct remove_reference<_Tp &&> { typedef _Tp type; };
-
-template 
-typename remove_reference<_Tp>::type &(_Tp &&__t) {
-  return static_cast::type &&>(__t);
-}
-
-template 
-_Tp &(typename remove_reference<_Tp>::type &__t) noexcept {
-  return static_cast<_Tp &&>(__t);
-}
-
-template 
-void swap(T , T ) {
-  T c(std::move(a));
-  a = std::move(b);
-  b = std::move(c);
-}
-
-template 
-class vector {
-public:
-  vector();
-  void push_back(const T );
-};
-
-} // namespace std
+#include "Inputs/system-header-simulator-cxx.h"
 

This is cool, thanks!


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55307/new/

https://reviews.llvm.org/D55307



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added reviewers: dcoughlin, xazax.hun, a_sidorin, george.karpenkov, szepet, 
rnkovacs, Szelethus.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, mikhail.ramalho, 
a.sidorin, baloghadamsoftware.

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.


Repository:
  rC Clang

https://reviews.llvm.org/D55307

Files:
  lib/StaticAnalyzer/Checkers/MoveChecker.cpp
  test/Analysis/Inputs/system-header-simulator-cxx.h
  test/Analysis/diagnostics/explicit-suppression.cpp
  test/Analysis/use-after-move.cpp

Index: test/Analysis/use-after-move.cpp
===
--- test/Analysis/use-after-move.cpp
+++ test/Analysis/use-after-move.cpp
@@ -13,47 +13,7 @@
 // RUN:  -analyzer-config exploration_strategy=dfs -DDFS=1\
 // RUN:  -analyzer-config alpha.cplusplus.Move:Aggressive=true -DAGGRESSIVE
 
-namespace std {
-
-typedef __typeof(sizeof(int)) size_t;
-
-template 
-struct remove_reference;
-
-template 
-struct remove_reference { typedef _Tp type; };
-
-template 
-struct remove_reference<_Tp &> { typedef _Tp type; };
-
-template 
-struct remove_reference<_Tp &&> { typedef _Tp type; };
-
-template 
-typename remove_reference<_Tp>::type &(_Tp &&__t) {
-  return static_cast::type &&>(__t);
-}
-
-template 
-_Tp &(typename remove_reference<_Tp>::type &__t) noexcept {
-  return static_cast<_Tp &&>(__t);
-}
-
-template 
-void swap(T , T ) {
-  T c(std::move(a));
-  a = std::move(b);
-  b = std::move(c);
-}
-
-template 
-class vector {
-public:
-  vector();
-  void push_back(const T );
-};
-
-} // namespace std
+#include "Inputs/system-header-simulator-cxx.h"
 
 class B {
 public:
@@ -832,13 +792,26 @@
 
 class HasSTLField {
   std::vector V;
-  void foo() {
+  void testVector() {
 // Warn even in non-aggressive mode when it comes to STL, because
 // in STL the object is left in "valid but unspecified state" after move.
 std::vector W = std::move(V); // expected-note{{Object 'V' of type 'std::vector' is left in a valid but unspecified state after move}}
 V.push_back(123); // expected-warning{{Method called on moved-from object 'V'}}
   // expected-note@-1{{Method called on moved-from object 'V'}}
   }
+
+  std::unique_ptr P;
+  void testUniquePtr() {
+// unique_ptr remains in a well-defined state after move.
+std::unique_ptr Q = std::move(P);
+P.get();
+#ifdef AGGRESSIVE
+// expected-warning@-2{{Method called on moved-from object 'P'}}
+// expected-note@-4{{Object 'P' is moved}}
+// expected-note@-4{{Method called on moved-from object 'P'}}
+#endif
+*P += 1; // FIXME: Should warn that the pointer is null.
+  }
 };
 
 void localRValueMove(A &) {
@@ -846,3 +819,11 @@
   a.foo(); // expected-warning{{Method called on moved-from object 'a'}}
// expected-note@-1{{Method called on moved-from object 'a'}}
 }
+
+void localUniquePtr(std::unique_ptr P) {
+  // Even though unique_ptr is safe to use after move,
+  // reusing a local variable this way usually indicates a bug.
+  std::unique_ptr Q = std::move(P); // expected-note{{Object 'P' is moved}}
+  P.get(); // expected-warning{{Method called on moved-from object 'P'}}
+   // expected-note@-1{{Method called on moved-from object 'P'}}
+}
Index: test/Analysis/diagnostics/explicit-suppression.cpp
===
--- test/Analysis/diagnostics/explicit-suppression.cpp
+++ test/Analysis/diagnostics/explicit-suppression.cpp
@@ -19,6 +19,6 @@
 void testCopyNull(C *I, C *E) {
   std::copy(I, E, (C *)0);
 #ifndef SUPPRESSED
-  // expected-warning@../Inputs/system-header-simulator-cxx.h:670 {{Called C++ object pointer is null}}
+  // expected-warning@../Inputs/system-header-simulator-cxx.h:677 {{Called C++ object pointer is null}}
 #endif
 }
Index: test/Analysis/Inputs/system-header-simulator-cxx.h
===
--- test/Analysis/Inputs/system-header-simulator-cxx.h
+++ test/Analysis/Inputs/system-header-simulator-cxx.h
@@ -237,6 +237,13 @@
 return static_cast(a);
   }
 
+  template 
+  void swap(T , T ) {
+T c(std::move(a));
+a = std::move(b);
+b = std::move(c);
+  }
+
   template
   class vector {
 typedef T value_type;
@@ -770,6 +777,22 @@
 
 }
 
+#if __cplusplus >= 201103L
+namespace std {
+  template // TODO: Implement the stub for deleter.
+  class unique_ptr {
+  public:
+unique_ptr(const unique_ptr &) = delete;
+unique_ptr(unique_ptr &&);
+
+T *get() const;
+
+typename std::add_lvalue_reference::type operator*() const;
+T