[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun updated this revision to Diff 134802.
khuttun added a comment.

I changed the checker to use hasAnyName. Checking for `unique_ptr::release()` 
and all the `empty()` functions is removed.

The checker doesn't report any warnings from LLVM + clang codebases now.


https://reviews.llvm.org/D41655

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/UnusedReturnValueCheck.cpp
  clang-tidy/bugprone/UnusedReturnValueCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-unused-return-value.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-unused-return-value-custom.cpp
  test/clang-tidy/bugprone-unused-return-value.cpp

Index: test/clang-tidy/bugprone-unused-return-value.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-unused-return-value.cpp
@@ -0,0 +1,166 @@
+// RUN: %check_clang_tidy %s bugprone-unused-return-value %t
+
+namespace std {
+
+struct future {};
+
+enum class launch {
+  async,
+  deferred
+};
+
+template 
+future async(Function &&, Args &&...);
+
+template 
+future async(launch, Function &&, Args &&...);
+
+template 
+ForwardIt remove(ForwardIt, ForwardIt, const T &);
+
+template 
+ForwardIt remove_if(ForwardIt, ForwardIt, UnaryPredicate);
+
+template 
+ForwardIt unique(ForwardIt, ForwardIt);
+
+// the check should be able to match std lib calls even if the functions are
+// declared inside inline namespaces
+inline namespace v1 {
+
+template 
+T *launder(T *);
+
+} // namespace v1
+} // namespace std
+
+struct Foo {
+  void f();
+};
+
+int increment(int i) {
+  return i + 1;
+}
+
+void useFuture(const std::future &fut);
+
+void warning() {
+  std::async(increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::async(std::launch::async, increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  Foo F;
+  std::launder(&F);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::remove_if(nullptr, nullptr, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::unique(nullptr, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  // test discarding return values inside different kinds of statements
+
+  auto Lambda = [] { std::remove(nullptr, nullptr, 1); };
+  // CHECK-MESSAGES: [[@LINE-1]]:22: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  if (true)
+std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  else if (true)
+std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  else
+std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  while (true)
+std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  do
+std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  while (true);
+
+  for (;;)
+std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  for (std::remove(nullptr, nullptr, 1);;)
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: the value returned by this function should be used [bugprone-unused-return-value]
+;
+
+  for (;; std::remove(nullptr, nullptr, 1))
+// CHECK-MESSAGES: [[@LINE-1]]:11: warning: the value returned by this function should be used [bugprone-unused-return-value]
+;
+
+  for (auto C : "foo")
+std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  switch (1) {
+  case 1:
+std::remove(nullptr, nullptr, 1);
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+break;
+  default:
+std::remove(nullptr, nullptr, 1);
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by 

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun created this revision.
khuttun added reviewers: alexfh, aaron.ballman.
khuttun added a project: clang-tools-extra.
Herald added subscribers: xazax.hun, mgorny.

Detects function calls where the return value is unused.

Checked functions can be configured.


https://reviews.llvm.org/D41655

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/UnusedReturnValueCheck.cpp
  clang-tidy/bugprone/UnusedReturnValueCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-unused-return-value.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-unused-return-value-custom.cpp
  test/clang-tidy/bugprone-unused-return-value.cpp

Index: test/clang-tidy/bugprone-unused-return-value.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-unused-return-value.cpp
@@ -0,0 +1,200 @@
+// RUN: %check_clang_tidy %s bugprone-unused-return-value %t
+
+namespace std {
+
+using size_t = decltype(sizeof(int));
+
+using max_align_t = long double;
+
+struct future {};
+
+enum class launch {
+  async,
+  deferred
+};
+
+template 
+future async(Function &&, Args &&...);
+
+template 
+future async(launch, Function &&, Args &&...);
+
+// the check should be able to match std lib calls even if the functions are
+// declared inside inline namespaces
+inline namespace v1 {
+
+template 
+T *launder(T *);
+
+} // namespace v1
+
+template 
+struct allocator {
+  using value_type = T;
+  T *allocate(std::size_t);
+  T *allocate(std::size_t, const void *);
+};
+
+template 
+struct allocator_traits {
+  using value_type = typename Alloc::value_type;
+  using pointer = value_type *;
+  using size_type = size_t;
+  using const_void_pointer = const void *;
+  static pointer allocate(Alloc &, size_type);
+  static pointer allocate(Alloc &, size_type, const_void_pointer);
+};
+
+template 
+struct scoped_allocator_adaptor : public OuterAlloc {
+  using pointer = typename allocator_traits::pointer;
+  using size_type = typename allocator_traits::size_type;
+  using const_void_pointer = typename allocator_traits::const_void_pointer;
+  pointer allocate(size_type);
+  pointer allocate(size_type, const_void_pointer);
+};
+
+template 
+struct default_delete {};
+
+template >
+struct unique_ptr {
+  using pointer = T *;
+  pointer release() noexcept;
+};
+
+template >
+struct vector {
+  bool empty() const noexcept;
+};
+
+namespace filesystem {
+
+struct path {
+  bool empty() const noexcept;
+};
+
+} // namespace filesystem
+
+namespace pmr {
+
+struct memory_resource {
+  void *allocate(size_t, size_t = alignof(max_align_t));
+};
+
+template 
+struct polymorphic_allocator {
+  T *allocate(size_t);
+};
+
+} // namespace pmr
+
+} // namespace std
+
+struct Foo {
+  void f();
+};
+
+int increment(int i) {
+  return i + 1;
+}
+
+void useFuture(const std::future &fut);
+
+struct FooAlloc {
+  using value_type = Foo;
+};
+
+void warning() {
+  std::async(increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  std::async(std::launch::async, increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  Foo F;
+  std::launder(&F);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  std::unique_ptr UP;
+  UP.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  std::allocator FA;
+  FA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+  FA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  std::allocator_traits>::allocate(FA, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+  std::allocator_traits>::allocate(FA, 1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  std::scoped_allocator_adaptor SAA;
+  SAA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+  SAA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  std::pmr::memory_resource MR;
+  MR.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  std::pmr::polymorphic_allocator PA;
+  PA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  std::vector FV;
+  FV.empty();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  std::filesystem::path P;
+  P.empty();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+}
+
+void noWarning() {
+  auto AsyncRetval1 = std

[PATCH] D41056: [clang-tidy] New check misc-uniqueptr-release-unused-retval

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun abandoned this revision.
khuttun added a comment.

Closing this as more general check is being reviewed here: 
https://reviews.llvm.org/D41655


Repository:
  rL LLVM

https://reviews.llvm.org/D41056



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

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun marked 7 inline comments as done.
khuttun added a comment.

In https://reviews.llvm.org/D41655#965551, @JonasToth wrote:

> I think it would be more user friendly if the configured list can be a list 
> and the `|` concatenation is done within your code.


What do you exactly mean by list? It seems some other checks use comma or 
semicolon separators in their configuration strings, but is that really more 
user friendly than using "|"? At least now it's clear that the whole string is 
a regex.


https://reviews.llvm.org/D41655



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

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun updated this revision to Diff 128544.
khuttun added a comment.

Fix review comments


https://reviews.llvm.org/D41655

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/UnusedReturnValueCheck.cpp
  clang-tidy/bugprone/UnusedReturnValueCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-unused-return-value.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-unused-return-value-custom.cpp
  test/clang-tidy/bugprone-unused-return-value.cpp

Index: test/clang-tidy/bugprone-unused-return-value.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-unused-return-value.cpp
@@ -0,0 +1,208 @@
+// RUN: %check_clang_tidy %s bugprone-unused-return-value %t
+
+namespace std {
+
+using size_t = decltype(sizeof(int));
+
+using max_align_t = long double;
+
+struct future {};
+
+enum class launch {
+  async,
+  deferred
+};
+
+template 
+future async(Function &&, Args &&...);
+
+template 
+future async(launch, Function &&, Args &&...);
+
+template 
+bool empty(const T&);
+
+// the check should be able to match std lib calls even if the functions are
+// declared inside inline namespaces
+inline namespace v1 {
+
+template 
+T *launder(T *);
+
+} // namespace v1
+
+template 
+struct allocator {
+  using value_type = T;
+  T *allocate(std::size_t);
+  T *allocate(std::size_t, const void *);
+};
+
+template 
+struct allocator_traits {
+  using value_type = typename Alloc::value_type;
+  using pointer = value_type *;
+  using size_type = size_t;
+  using const_void_pointer = const void *;
+  static pointer allocate(Alloc &, size_type);
+  static pointer allocate(Alloc &, size_type, const_void_pointer);
+};
+
+template 
+struct scoped_allocator_adaptor : public OuterAlloc {
+  using pointer = typename allocator_traits::pointer;
+  using size_type = typename allocator_traits::size_type;
+  using const_void_pointer = typename allocator_traits::const_void_pointer;
+  pointer allocate(size_type);
+  pointer allocate(size_type, const_void_pointer);
+};
+
+template 
+struct default_delete {};
+
+template >
+struct unique_ptr {
+  using pointer = T *;
+  pointer release() noexcept;
+};
+
+template >
+struct vector {
+  bool empty() const noexcept;
+};
+
+namespace filesystem {
+
+struct path {
+  bool empty() const noexcept;
+};
+
+} // namespace filesystem
+
+namespace pmr {
+
+struct memory_resource {
+  void *allocate(size_t, size_t = alignof(max_align_t));
+};
+
+template 
+struct polymorphic_allocator {
+  T *allocate(size_t);
+};
+
+} // namespace pmr
+
+} // namespace std
+
+struct Foo {
+  void f();
+};
+
+int increment(int i) {
+  return i + 1;
+}
+
+void useFuture(const std::future &fut);
+
+struct FooAlloc {
+  using value_type = Foo;
+};
+
+void warning() {
+  std::async(increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'async' should normally be used [bugprone-unused-return-value]
+
+  std::async(std::launch::async, increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'async' should normally be used [bugprone-unused-return-value]
+
+  Foo F;
+  std::launder(&F);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'launder' should normally be used [bugprone-unused-return-value]
+
+  std::unique_ptr UP;
+  UP.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'release' should normally be used [bugprone-unused-return-value]
+
+  std::allocator FA;
+  FA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'allocate' should normally be used [bugprone-unused-return-value]
+  FA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'allocate' should normally be used [bugprone-unused-return-value]
+
+  std::allocator_traits>::allocate(FA, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'allocate' should normally be used [bugprone-unused-return-value]
+  std::allocator_traits>::allocate(FA, 1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'allocate' should normally be used [bugprone-unused-return-value]
+
+  std::scoped_allocator_adaptor SAA;
+  SAA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'allocate' should normally be used [bugprone-unused-return-value]
+  SAA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'allocate' should normally be used [bugprone-unused-return-value]
+
+  std::pmr::memory_resource MR;
+  MR.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'allocate' should normally be used [bugprone-unused-return-value]
+
+  std::pmr::polymorphic_allocator PA;
+  PA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'allocate' should normally be used [bugprone-unused-return-value]
+
+  std::vector FV;
+  F

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun marked an inline comment as done.
khuttun added inline comments.



Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:69
+ "the value returned by %0 should normally be used")
+<< dyn_cast_or_null(Matched->getCalleeDecl())
+<< Matched->getSourceRange();

aaron.ballman wrote:
> In the event this returns null, the diagnostic is going to look rather odd. 
> Because the value of the call expression is unused, this will most often 
> trigger in a context where the method call can be inferred (especially 
> because you're now highlighting the source range). It might make sense to 
> simply replace the %0 with "this call expression" or somesuch in the 
> diagnostic.
I can remove the function name from the diagnostic. Out of curiosity, in which 
situations could it be null?


https://reviews.llvm.org/D41655



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

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun updated this revision to Diff 128844.
khuttun added a comment.

Fix review comments


https://reviews.llvm.org/D41655

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/UnusedReturnValueCheck.cpp
  clang-tidy/bugprone/UnusedReturnValueCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-unused-return-value.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-unused-return-value-custom.cpp
  test/clang-tidy/bugprone-unused-return-value.cpp

Index: test/clang-tidy/bugprone-unused-return-value.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-unused-return-value.cpp
@@ -0,0 +1,208 @@
+// RUN: %check_clang_tidy %s bugprone-unused-return-value %t
+
+namespace std {
+
+using size_t = decltype(sizeof(int));
+
+using max_align_t = long double;
+
+struct future {};
+
+enum class launch {
+  async,
+  deferred
+};
+
+template 
+future async(Function &&, Args &&...);
+
+template 
+future async(launch, Function &&, Args &&...);
+
+template 
+bool empty(const T&);
+
+// the check should be able to match std lib calls even if the functions are
+// declared inside inline namespaces
+inline namespace v1 {
+
+template 
+T *launder(T *);
+
+} // namespace v1
+
+template 
+struct allocator {
+  using value_type = T;
+  T *allocate(std::size_t);
+  T *allocate(std::size_t, const void *);
+};
+
+template 
+struct allocator_traits {
+  using value_type = typename Alloc::value_type;
+  using pointer = value_type *;
+  using size_type = size_t;
+  using const_void_pointer = const void *;
+  static pointer allocate(Alloc &, size_type);
+  static pointer allocate(Alloc &, size_type, const_void_pointer);
+};
+
+template 
+struct scoped_allocator_adaptor : public OuterAlloc {
+  using pointer = typename allocator_traits::pointer;
+  using size_type = typename allocator_traits::size_type;
+  using const_void_pointer = typename allocator_traits::const_void_pointer;
+  pointer allocate(size_type);
+  pointer allocate(size_type, const_void_pointer);
+};
+
+template 
+struct default_delete {};
+
+template >
+struct unique_ptr {
+  using pointer = T *;
+  pointer release() noexcept;
+};
+
+template >
+struct vector {
+  bool empty() const noexcept;
+};
+
+namespace filesystem {
+
+struct path {
+  bool empty() const noexcept;
+};
+
+} // namespace filesystem
+
+namespace pmr {
+
+struct memory_resource {
+  void *allocate(size_t, size_t = alignof(max_align_t));
+};
+
+template 
+struct polymorphic_allocator {
+  T *allocate(size_t);
+};
+
+} // namespace pmr
+
+} // namespace std
+
+struct Foo {
+  void f();
+};
+
+int increment(int i) {
+  return i + 1;
+}
+
+void useFuture(const std::future &fut);
+
+struct FooAlloc {
+  using value_type = Foo;
+};
+
+void warning() {
+  std::async(increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::async(std::launch::async, increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  Foo F;
+  std::launder(&F);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::unique_ptr UP;
+  UP.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::allocator FA;
+  FA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  FA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::allocator_traits>::allocate(FA, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  std::allocator_traits>::allocate(FA, 1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::scoped_allocator_adaptor SAA;
+  SAA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  SAA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::pmr::memory_resource MR;
+  MR.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::pmr::polymorphic_allocator PA;
+  PA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::vector FV;
+  FV.empty();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the val

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

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



Comment at: test/clang-tidy/bugprone-unused-return-value.cpp:163
+
+void noWarning() {
+  auto AsyncRetval1 = std::async(increment, 42);

aaron.ballman wrote:
> Sorry, I just realized that we're missing a test case for a common situation 
> -- where the result is used as part of another call expression. Can you add a 
> test to `noWarning()` to make sure this does not warn:
> ```
> std::vector v;
> extern void f(bool);
> 
> f(v.empty()); // Should not warn
> ```
See line 199 in this file.


https://reviews.llvm.org/D41655



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

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun updated this revision to Diff 128879.
khuttun added a comment.

Fix review comments


https://reviews.llvm.org/D41655

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/UnusedReturnValueCheck.cpp
  clang-tidy/bugprone/UnusedReturnValueCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-unused-return-value.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-unused-return-value-custom.cpp
  test/clang-tidy/bugprone-unused-return-value.cpp

Index: test/clang-tidy/bugprone-unused-return-value.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-unused-return-value.cpp
@@ -0,0 +1,208 @@
+// RUN: %check_clang_tidy %s bugprone-unused-return-value %t
+
+namespace std {
+
+using size_t = decltype(sizeof(int));
+
+using max_align_t = long double;
+
+struct future {};
+
+enum class launch {
+  async,
+  deferred
+};
+
+template 
+future async(Function &&, Args &&...);
+
+template 
+future async(launch, Function &&, Args &&...);
+
+template 
+bool empty(const T&);
+
+// the check should be able to match std lib calls even if the functions are
+// declared inside inline namespaces
+inline namespace v1 {
+
+template 
+T *launder(T *);
+
+} // namespace v1
+
+template 
+struct allocator {
+  using value_type = T;
+  T *allocate(std::size_t);
+  T *allocate(std::size_t, const void *);
+};
+
+template 
+struct allocator_traits {
+  using value_type = typename Alloc::value_type;
+  using pointer = value_type *;
+  using size_type = size_t;
+  using const_void_pointer = const void *;
+  static pointer allocate(Alloc &, size_type);
+  static pointer allocate(Alloc &, size_type, const_void_pointer);
+};
+
+template 
+struct scoped_allocator_adaptor : public OuterAlloc {
+  using pointer = typename allocator_traits::pointer;
+  using size_type = typename allocator_traits::size_type;
+  using const_void_pointer = typename allocator_traits::const_void_pointer;
+  pointer allocate(size_type);
+  pointer allocate(size_type, const_void_pointer);
+};
+
+template 
+struct default_delete {};
+
+template >
+struct unique_ptr {
+  using pointer = T *;
+  pointer release() noexcept;
+};
+
+template >
+struct vector {
+  bool empty() const noexcept;
+};
+
+namespace filesystem {
+
+struct path {
+  bool empty() const noexcept;
+};
+
+} // namespace filesystem
+
+namespace pmr {
+
+struct memory_resource {
+  void *allocate(size_t, size_t = alignof(max_align_t));
+};
+
+template 
+struct polymorphic_allocator {
+  T *allocate(size_t);
+};
+
+} // namespace pmr
+
+} // namespace std
+
+struct Foo {
+  void f();
+};
+
+int increment(int i) {
+  return i + 1;
+}
+
+void useFuture(const std::future &fut);
+
+struct FooAlloc {
+  using value_type = Foo;
+};
+
+void warning() {
+  std::async(increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::async(std::launch::async, increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  Foo F;
+  std::launder(&F);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::unique_ptr UP;
+  UP.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::allocator FA;
+  FA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  FA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::allocator_traits>::allocate(FA, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  std::allocator_traits>::allocate(FA, 1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::scoped_allocator_adaptor SAA;
+  SAA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  SAA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::pmr::memory_resource MR;
+  MR.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::pmr::polymorphic_allocator PA;
+  PA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::vector FV;
+  FV.empty();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the val

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added inline comments.



Comment at: test/clang-tidy/bugprone-unused-return-value.cpp:163
+
+void noWarning() {
+  auto AsyncRetval1 = std::async(increment, 42);

aaron.ballman wrote:
> khuttun wrote:
> > aaron.ballman wrote:
> > > Sorry, I just realized that we're missing a test case for a common 
> > > situation -- where the result is used as part of another call expression. 
> > > Can you add a test to `noWarning()` to make sure this does not warn:
> > > ```
> > > std::vector v;
> > > extern void f(bool);
> > > 
> > > f(v.empty()); // Should not warn
> > > ```
> > See line 199 in this file.
> Ah, my eyes missed that, thank you!
> 
> Hmm, I *think* this test should also be okay, but I want to be sure:
> ```
> std::vector v;
> bool b = ({v.empty()}); // Should not warn
> ({v.empty()}); // ???
> ```
> I kind of thing that, as an extension to the spirit of this check, any GNU 
> expression statement whose result is unused should probably be diagnosed; 
> what do you think?
> 
> You should add tests for the above so we document the expected behavior here.
Getting a warning when just surrounding the call expression with parenthesis is 
tested in bugprone-unused-return-value-custom.cpp.

Would your example be parsed as creating an initializer_list? In that case it 
should not warn. I can add test cases for that.

What do you mean by "GNU expression"?


https://reviews.llvm.org/D41655



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

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added inline comments.



Comment at: test/clang-tidy/bugprone-unused-return-value.cpp:163
+
+void noWarning() {
+  auto AsyncRetval1 = std::async(increment, 42);

aaron.ballman wrote:
> khuttun wrote:
> > aaron.ballman wrote:
> > > khuttun wrote:
> > > > aaron.ballman wrote:
> > > > > Sorry, I just realized that we're missing a test case for a common 
> > > > > situation -- where the result is used as part of another call 
> > > > > expression. Can you add a test to `noWarning()` to make sure this 
> > > > > does not warn:
> > > > > ```
> > > > > std::vector v;
> > > > > extern void f(bool);
> > > > > 
> > > > > f(v.empty()); // Should not warn
> > > > > ```
> > > > See line 199 in this file.
> > > Ah, my eyes missed that, thank you!
> > > 
> > > Hmm, I *think* this test should also be okay, but I want to be sure:
> > > ```
> > > std::vector v;
> > > bool b = ({v.empty()}); // Should not warn
> > > ({v.empty()}); // ???
> > > ```
> > > I kind of thing that, as an extension to the spirit of this check, any 
> > > GNU expression statement whose result is unused should probably be 
> > > diagnosed; what do you think?
> > > 
> > > You should add tests for the above so we document the expected behavior 
> > > here.
> > Getting a warning when just surrounding the call expression with 
> > parenthesis is tested in bugprone-unused-return-value-custom.cpp.
> > 
> > Would your example be parsed as creating an initializer_list? In that case 
> > it should not warn. I can add test cases for that.
> > 
> > What do you mean by "GNU expression"?
> > Getting a warning when just surrounding the call expression with 
> > parenthesis is tested in bugprone-unused-return-value-custom.cpp.
> 
> Yup, this is something entirely different, however.
> 
> > Would your example be parsed as creating an initializer_list? In that case 
> > it should not warn. I can add test cases for that.
> >
> > What do you mean by "GNU expression"?
> 
> Those examples are GNU expression statements, which is a GCC extension that 
> Clang also supports. See 
> https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html for more information.
> 
> Effectively, the last statement in the GNU expression statement is the 
> "return value" of the expression statement. We should make sure the warnings 
> are sensible with this construct.
This was a good catch. Code like this

```
auto Gnu = ({ fun(); });
```

is currently causing a warning.


https://reviews.llvm.org/D41655



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

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun updated this revision to Diff 129090.
khuttun added a comment.

The checker is now disabled inside GNU statement expressions


https://reviews.llvm.org/D41655

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/UnusedReturnValueCheck.cpp
  clang-tidy/bugprone/UnusedReturnValueCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-unused-return-value.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-unused-return-value-custom.cpp
  test/clang-tidy/bugprone-unused-return-value.cpp

Index: test/clang-tidy/bugprone-unused-return-value.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-unused-return-value.cpp
@@ -0,0 +1,212 @@
+// RUN: %check_clang_tidy %s bugprone-unused-return-value %t
+
+namespace std {
+
+using size_t = decltype(sizeof(int));
+
+using max_align_t = long double;
+
+struct future {};
+
+enum class launch {
+  async,
+  deferred
+};
+
+template 
+future async(Function &&, Args &&...);
+
+template 
+future async(launch, Function &&, Args &&...);
+
+template 
+bool empty(const T &);
+
+// the check should be able to match std lib calls even if the functions are
+// declared inside inline namespaces
+inline namespace v1 {
+
+template 
+T *launder(T *);
+
+} // namespace v1
+
+template 
+struct allocator {
+  using value_type = T;
+  T *allocate(std::size_t);
+  T *allocate(std::size_t, const void *);
+};
+
+template 
+struct allocator_traits {
+  using value_type = typename Alloc::value_type;
+  using pointer = value_type *;
+  using size_type = size_t;
+  using const_void_pointer = const void *;
+  static pointer allocate(Alloc &, size_type);
+  static pointer allocate(Alloc &, size_type, const_void_pointer);
+};
+
+template 
+struct scoped_allocator_adaptor : public OuterAlloc {
+  using pointer = typename allocator_traits::pointer;
+  using size_type = typename allocator_traits::size_type;
+  using const_void_pointer = typename allocator_traits::const_void_pointer;
+  pointer allocate(size_type);
+  pointer allocate(size_type, const_void_pointer);
+};
+
+template 
+struct default_delete {};
+
+template >
+struct unique_ptr {
+  using pointer = T *;
+  pointer release() noexcept;
+};
+
+template >
+struct vector {
+  bool empty() const noexcept;
+};
+
+namespace filesystem {
+
+struct path {
+  bool empty() const noexcept;
+};
+
+} // namespace filesystem
+
+namespace pmr {
+
+struct memory_resource {
+  void *allocate(size_t, size_t = alignof(max_align_t));
+};
+
+template 
+struct polymorphic_allocator {
+  T *allocate(size_t);
+};
+
+} // namespace pmr
+
+} // namespace std
+
+struct Foo {
+  void f();
+};
+
+int increment(int i) {
+  return i + 1;
+}
+
+void useFuture(const std::future &fut);
+
+struct FooAlloc {
+  using value_type = Foo;
+};
+
+void warning() {
+  std::async(increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::async(std::launch::async, increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  Foo F;
+  std::launder(&F);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::unique_ptr UP;
+  UP.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::allocator FA;
+  FA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  FA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::allocator_traits>::allocate(FA, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  std::allocator_traits>::allocate(FA, 1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::scoped_allocator_adaptor SAA;
+  SAA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  SAA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::pmr::memory_resource MR;
+  MR.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::pmr::polymorphic_allocator PA;
+  PA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::vector FV;
+  FV.empty();
+  // CHECK

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

Should I close this review?


https://reviews.llvm.org/D41655



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

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

In https://reviews.llvm.org/D41655#1037234, @alexfh wrote:

> Do you need help committing the patch?


Yes please, I don't have commit access to the repo.

I think the next step for improving this checker could be to make it work with 
class template member functions. That would allow to add checking also to those 
container functions that https://reviews.llvm.org/D17043 handles.


https://reviews.llvm.org/D41655



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

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun updated this revision to Diff 138595.
khuttun added a comment.

Rebased


https://reviews.llvm.org/D41655

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/UnusedReturnValueCheck.cpp
  clang-tidy/bugprone/UnusedReturnValueCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-unused-return-value.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-unused-return-value-custom.cpp
  test/clang-tidy/bugprone-unused-return-value.cpp

Index: test/clang-tidy/bugprone-unused-return-value.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-unused-return-value.cpp
@@ -0,0 +1,166 @@
+// RUN: %check_clang_tidy %s bugprone-unused-return-value %t
+
+namespace std {
+
+struct future {};
+
+enum class launch {
+  async,
+  deferred
+};
+
+template 
+future async(Function &&, Args &&...);
+
+template 
+future async(launch, Function &&, Args &&...);
+
+template 
+ForwardIt remove(ForwardIt, ForwardIt, const T &);
+
+template 
+ForwardIt remove_if(ForwardIt, ForwardIt, UnaryPredicate);
+
+template 
+ForwardIt unique(ForwardIt, ForwardIt);
+
+// the check should be able to match std lib calls even if the functions are
+// declared inside inline namespaces
+inline namespace v1 {
+
+template 
+T *launder(T *);
+
+} // namespace v1
+} // namespace std
+
+struct Foo {
+  void f();
+};
+
+int increment(int i) {
+  return i + 1;
+}
+
+void useFuture(const std::future &fut);
+
+void warning() {
+  std::async(increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::async(std::launch::async, increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  Foo F;
+  std::launder(&F);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::remove_if(nullptr, nullptr, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::unique(nullptr, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  // test discarding return values inside different kinds of statements
+
+  auto Lambda = [] { std::remove(nullptr, nullptr, 1); };
+  // CHECK-MESSAGES: [[@LINE-1]]:22: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  if (true)
+std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  else if (true)
+std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  else
+std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  while (true)
+std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  do
+std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  while (true);
+
+  for (;;)
+std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  for (std::remove(nullptr, nullptr, 1);;)
+// CHECK-MESSAGES: [[@LINE-1]]:8: warning: the value returned by this function should be used [bugprone-unused-return-value]
+;
+
+  for (;; std::remove(nullptr, nullptr, 1))
+// CHECK-MESSAGES: [[@LINE-1]]:11: warning: the value returned by this function should be used [bugprone-unused-return-value]
+;
+
+  for (auto C : "foo")
+std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  switch (1) {
+  case 1:
+std::remove(nullptr, nullptr, 1);
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+break;
+  default:
+std::remove(nullptr, nullptr, 1);
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+break;
+  }
+
+  try {
+std::remove(nullptr, nullptr, 1);
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value retu

[PATCH] D59103: [clang-tidy] New checker bugprone-incomplete-comparison-operator

[Kalle Huttunen via Phabricator via cfe-commits]

kallehuttunen created this revision.
kallehuttunen added reviewers: alexfh, hokein, JonasToth, aaron.ballman.
kallehuttunen added a project: clang-tools-extra.
Herald added subscribers: jdoerfert, xazax.hun, mgorny.
Herald added a project: clang.

The checker detects comparison operators that don't access all fields of the 
compared types.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D59103

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/IncompleteComparisonOperatorCheck.cpp
  clang-tidy/bugprone/IncompleteComparisonOperatorCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-incomplete-comparison-operator.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-incomplete-comparison-operator-custom.cpp
  test/clang-tidy/bugprone-incomplete-comparison-operator.cpp

Index: test/clang-tidy/bugprone-incomplete-comparison-operator.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-incomplete-comparison-operator.cpp
@@ -0,0 +1,280 @@
+// RUN: %check_clang_tidy %s bugprone-incomplete-comparison-operator %t
+
+struct Good {
+  int Member1;
+  int Member2;
+};
+
+bool operator==(const Good &Lhs, const Good &Rhs) {
+  return Lhs.Member1 == Rhs.Member1 && Lhs.Member2 == Rhs.Member2;
+}
+
+bool operator!=(const Good &Lhs, const Good &Rhs) {
+  return !(Lhs == Rhs);
+}
+
+bool operator<(const Good &Lhs, const Good &Rhs) {
+  if (Lhs.Member1 != Rhs.Member1)
+return Lhs.Member1 < Rhs.Member1;
+  return Lhs.Member2 < Rhs.Member2;
+}
+
+bool operator>(const Good &Lhs, const Good &Rhs) {
+  return Rhs < Lhs;
+}
+
+bool operator<=(const Good &Lhs, const Good &Rhs) {
+  return !(Rhs < Lhs);
+}
+
+bool operator>=(const Good &Lhs, const Good &Rhs) {
+  return !(Lhs < Rhs);
+}
+
+struct Bad {
+  int Member1;
+  int Member2;
+};
+
+bool operator==(const Bad &Lhs, const Bad &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Lhs.Member2 not accessed
+  // CHECK-NOTES: [[@LINE-3]]:1: note: Rhs.Member2 not accessed
+  return Lhs.Member1 == Rhs.Member1;
+}
+
+bool operator!=(const Bad &Lhs, const Bad &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Lhs.Member2 not accessed
+  // CHECK-NOTES: [[@LINE-3]]:1: note: Rhs.Member2 not accessed
+  return !(Lhs == Rhs);
+}
+
+bool operator<(const Bad &Lhs, const Bad &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Lhs.Member1 not accessed
+  // CHECK-NOTES: [[@LINE-3]]:1: note: Rhs.Member1 not accessed
+  return Lhs.Member2 < Rhs.Member2;
+}
+
+bool operator>(const Bad &Lhs, const Bad &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Lhs.Member1 not accessed
+  // CHECK-NOTES: [[@LINE-3]]:1: note: Rhs.Member1 not accessed
+  return Rhs < Lhs;
+}
+
+bool operator<=(const Bad &Lhs, const Bad &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Lhs.Member1 not accessed
+  // CHECK-NOTES: [[@LINE-3]]:1: note: Rhs.Member1 not accessed
+  return !(Rhs < Lhs);
+}
+
+bool operator>=(const Bad &Lhs, const Bad &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Lhs.Member1 not accessed
+  // CHECK-NOTES: [[@LINE-3]]:1: note: Rhs.Member1 not accessed
+  return !(Lhs < Rhs);
+}
+
+struct Ugly {
+  Good GoodParts;
+  Bad BadParts;
+};
+
+// Missing field access for only one of the params -> warning
+bool operator==(const Ugly &Lhs, const Ugly &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Rhs.BadParts not accessed
+  return Lhs.GoodParts == Rhs.GoodParts && Lhs.BadParts == Lhs.BadParts;
+}
+
+// Args are passed to a function in another TU -> no warning
+bool testForInequality(const Ugly &, const Ugly &);
+bool operator!=(const Ugly &Lhs, const Ugly &Rhs) {
+  return testForInequality(Lhs, Rhs);
+}
+
+// Fields are passed to a function in another TU, but some fields are not accessed -> warning
+bool compareGoodness(const Good &, const Good &);
+bool operator<(const Ugly &Lhs, const Ugly &Rhs) {
+  // CHECK-NOTES: [[@LINE-1]]:1: warning: incomplete comparison operator
+  // CHECK-NOTES: [[@LINE-2]]:1: note: Lhs.BadParts not accessed
+  // CHECK-NOTES: [[@LINE-3]]:1: note: Rhs.BadParts not accessed
+  return compareGoodness(Lhs.GoodParts, Rhs.GoodParts);
+}
+
+// Args are passed to recursive function -> no warning
+bool strangeLoop(const Ugly &U) { return strangeLoop(U); }
+bool operator>(const Ugly &Lhs, const Ugly &Rhs) {
+  return strangeLoop(Lhs);
+}
+
+class Encapsulated {
+public:
+  int get1() const { return Member1; }
+  int get2() const

[PATCH] D59103: [clang-tidy] New checker bugprone-incomplete-comparison-operator

[Kalle Huttunen via Phabricator via cfe-commits]

kallehuttunen added a comment.

The checker gives quite many warnings on LLVM code base. For example, running 
it for lib/Transforms/Scalar:

  /home/kalle/llvm/lib/Transforms/Scalar/GVN.cpp:119:3: warning: incomplete 
comparison operator [bugprone-incomplete-comparison-operator]
bool operator==(const Expression &other) const {
^
  /home/kalle/llvm/lib/Transforms/Scalar/GVN.cpp:119:3: note: commutative not 
accessed
  /home/kalle/llvm/lib/Transforms/Scalar/GVN.cpp:119:3: note: other.commutative 
not accessed
  
  /home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:151:3: warning: 
incomplete comparison operator [bugprone-incomplete-comparison-operator]
bool operator==(const CHIArg &A) { return VN == A.VN; }
^
  /home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:151:3: note: Dest not 
accessed
  /home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:151:3: note: I not 
accessed
  /home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:151:3: note: A.Dest not 
accessed
  /home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:151:3: note: A.I not 
accessed
  
  /home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:152:3: warning: 
incomplete comparison operator [bugprone-incomplete-comparison-operator]
bool operator!=(const CHIArg &A) { return !(*this == A); }
^
  /home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:152:3: note: Dest not 
accessed
  /home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:152:3: note: I not 
accessed
  /home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:152:3: note: A.Dest not 
accessed
  /home/kalle/llvm/lib/Transforms/Scalar/GVNHoist.cpp:152:3: note: A.I not 
accessed
  
  /home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: warning: incomplete 
comparison operator [bugprone-incomplete-comparison-operator]
bool operator>(const SinkingInstructionCandidate &Other) const {
^
  /home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: NumBlocks not 
accessed
  /home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: 
NumInstructions not accessed
  /home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: NumPHIs not 
accessed
  /home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: 
NumMemoryInsts not accessed
  /home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: Blocks not 
accessed
  /home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: 
Other.NumBlocks not accessed
  /home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: 
Other.NumInstructions not accessed
  /home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: Other.NumPHIs 
not accessed
  /home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: 
Other.NumMemoryInsts not accessed
  /home/kalle/llvm/lib/Transforms/Scalar/GVNSink.cpp:211:3: note: Other.Blocks 
not accessed
  
  /home/kalle/llvm/lib/Transforms/Scalar/MergeICmps.cpp:90:3: warning: 
incomplete comparison operator [bugprone-incomplete-comparison-operator]
bool operator<(const BCEAtom &O) const {
^
  /home/kalle/llvm/lib/Transforms/Scalar/MergeICmps.cpp:90:3: note: GEP not 
accessed
  /home/kalle/llvm/lib/Transforms/Scalar/MergeICmps.cpp:90:3: note: LoadI not 
accessed
  /home/kalle/llvm/lib/Transforms/Scalar/MergeICmps.cpp:90:3: note: O.GEP not 
accessed
  /home/kalle/llvm/lib/Transforms/Scalar/MergeICmps.cpp:90:3: note: O.LoadI not 
accessed
  
  /home/kalle/llvm/lib/Transforms/Scalar/NewGVN.cpp:436:3: warning: incomplete 
comparison operator [bugprone-incomplete-comparison-operator]
bool operator==(const Expression &Other) const {
^
  /home/kalle/llvm/lib/Transforms/Scalar/NewGVN.cpp:436:3: note: Other.Opcode 
not accessed
  /home/kalle/llvm/lib/Transforms/Scalar/NewGVN.cpp:436:3: note: Other.HashVal 
not accessed
  
  /home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:206:3: warning: incomplete 
comparison operator [bugprone-incomplete-comparison-operator]
friend LLVM_ATTRIBUTE_UNUSED bool operator<(const Slice &LHS,
^
  /home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:206:3: note: LHS.EndOffset 
not accessed
  /home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:206:3: note: 
LHS.UseAndIsSplittable not accessed
  
  /home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:210:3: warning: incomplete 
comparison operator [bugprone-incomplete-comparison-operator]
friend LLVM_ATTRIBUTE_UNUSED bool operator<(uint64_t LHSOffset,
^
  /home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:210:3: note: RHS.EndOffset 
not accessed
  /home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:210:3: note: 
RHS.UseAndIsSplittable not accessed
  
  /home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:582:3: warning: incomplete 
comparison operator [bugprone-incomplete-comparison-operator]
bool operator==(const partition_iterator &RHS) const {
^
  /home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:582:3: note: 
MaxSplitSliceEndOffset not accessed
  /home/kalle/llvm/lib/Transforms/Scalar/SROA.cpp:582:3: note: 
RHS.MaxSplitSliceEndOffset not accessed

Running the checker on 100k SLOC p

[PATCH] D59103: [clang-tidy] New checker bugprone-incomplete-comparison-operator

[Kalle Huttunen via Phabricator via cfe-commits]

kallehuttunen added a comment.

In D59103#1421875 , @lebedev.ri wrote:

> Thank you for working on this!
>
> This looks questionable to me.
>  Is this based on some coding standard?
>  Are there any numbers on the false-positive rate of theck?


It's not based on any coding standard, but is rather an attempt to eliminate 
one class of bugs (forgetting to update comparison operator when adding struct 
members) that I've encountered several times.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59103



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

[PATCH] D59103: [clang-tidy] New checker bugprone-incomplete-comparison-operator

[Kalle Huttunen via Phabricator via cfe-commits]

kallehuttunen added a comment.

I found this checker to be useful in the code base I initially developed it 
for, but the usage of comparison operators there is pretty much limited to 
comparing simple aggregate types. It's true that this checker can produce lots 
of false positives, maybe too much to be included to clang-tidy proper.

@JonasToth 's proposal is interesting. Overall I think the 
"modernize-use-defaulted-spaceship-op" check would be much more complex than 
this one, but I guess the `FieldAccessVisitor` could be used as a building 
block.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59103



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

[PATCH] D59103: [clang-tidy] New checker bugprone-incomplete-comparison-operator

[Kalle Huttunen via Phabricator via cfe-commits]

kallehuttunen added a comment.

Another idea that came to my mind would be to enable this check only for 
annotated types. So warning for missing field access would be only given for 
types that have for example `[[clang::annotate("value type")]]` annotation. 
Possibly other kinds of checks could be also developed for types that have the 
given annotation.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59103



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

[PATCH] D59103: [clang-tidy] New checker bugprone-incomplete-comparison-operator

[Kalle Huttunen via Phabricator via cfe-commits]

kallehuttunen added a comment.

In D59103#1423307 , @aaron.ballman 
wrote:

> In D59103#1422775 , @kallehuttunen 
> wrote:
>
> > Another idea that came to my mind would be to enable this check only for 
> > annotated types. So warning for missing field access would be only given 
> > for types that have for example `[[clang::annotate("value type")]]` 
> > annotation. Possibly other kinds of checks could be also developed for 
> > types that have the given annotation.
>
>
> I don't think `annotate` would be a good choice because the primary purpose 
> of that one is to pass that attribute information down to the backend and 
> using it for this purpose feels a bit hackish. However, we could always add a 
> new attribute if needed, but I'm not convinced an attribute is the right 
> approach either (but then again, I'm also lacking information I'm sure).
>
> What other kinds of checks do you have in mind and what are the semantics of 
> the attribute you're thinking of?


I was thinking about something along the lines of having an attribute to mark 
types to be a "value type" as defined in Herb's metaclasses proposal (chapter 
3.5 in https://herbsutter.files.wordpress.com/2017/07/p0707r1.pdf). There could 
be then checks for having default ctor, no virtual functions etc in addition to 
this check for those types. But then again now that I think about it, a 
"string" class is an counterexample for this: it can be a value (regular) type, 
but when comparing instances you don't want to access the "capacity" member.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D59103



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

[PATCH] D45891: [clang-tidy] Improve bugprone-unused-return-value check

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun created this revision.
khuttun added a reviewer: alexfh.
Herald added subscribers: cfe-commits, xazax.hun.

Add support for checking class template member functions.

Also add the following functions to be checked by default:

- std::unique_ptr::release
- std::basic_string::empty
- std::vector::empty


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45891

Files:
  clang-tidy/bugprone/UnusedReturnValueCheck.cpp
  docs/clang-tidy/checks/bugprone-unused-return-value.rst
  test/clang-tidy/bugprone-unused-return-value-custom.cpp
  test/clang-tidy/bugprone-unused-return-value.cpp

Index: test/clang-tidy/bugprone-unused-return-value.cpp
===
--- test/clang-tidy/bugprone-unused-return-value.cpp
+++ test/clang-tidy/bugprone-unused-return-value.cpp
@@ -24,6 +24,34 @@
 template 
 ForwardIt unique(ForwardIt, ForwardIt);
 
+template 
+struct default_delete;
+
+template >
+struct unique_ptr {
+  T *release() noexcept;
+};
+
+template 
+struct char_traits;
+
+template 
+struct allocator;
+
+template ,
+  typename Allocator = allocator>
+struct basic_string {
+  bool empty() const;
+};
+
+typedef basic_string string;
+
+template >
+struct vector {
+  bool empty() const noexcept;
+};
+
 // the check should be able to match std lib calls even if the functions are
 // declared inside inline namespaces
 inline namespace v1 {
@@ -64,6 +92,18 @@
   std::unique(nullptr, nullptr);
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
 
+  std::unique_ptr UPtr;
+  UPtr.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::string Str;
+  Str.empty();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::vector Vec;
+  Vec.empty();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
   // test discarding return values inside different kinds of statements
 
   auto Lambda = [] { std::remove(nullptr, nullptr, 1); };
@@ -137,6 +177,15 @@
 
   auto UniqueRetval = std::unique(nullptr, nullptr);
 
+  std::unique_ptr UPtrNoWarning;
+  auto ReleaseRetval = UPtrNoWarning.release();
+
+  std::string StrNoWarning;
+  auto StrEmptyRetval = StrNoWarning.empty();
+
+  std::vector VecNoWarning;
+  auto VecEmptyRetval = VecNoWarning.empty();
+
   // test using the return value in different kinds of expressions
   useFuture(std::async(increment, 42));
   std::launder(&FNoWarning)->f();
Index: test/clang-tidy/bugprone-unused-return-value-custom.cpp
===
--- test/clang-tidy/bugprone-unused-return-value-custom.cpp
+++ test/clang-tidy/bugprone-unused-return-value-custom.cpp
@@ -1,7 +1,7 @@
 // RUN: %check_clang_tidy %s bugprone-unused-return-value %t \
 // RUN: -config='{CheckOptions: \
 // RUN:  [{key: bugprone-unused-return-value.CheckedFunctions, \
-// RUN:value: "::fun;::ns::Outer::Inner::memFun;::ns::Type::staticFun"}]}' \
+// RUN:value: "::fun;::ns::Outer::Inner::memFun;::ns::Type::staticFun;::ns::ClassTemplate::memFun;::ns::ClassTemplate::staticFun"}]}' \
 // RUN: --
 
 namespace std {
@@ -34,6 +34,12 @@
   static Retval staticFun();
 };
 
+template 
+struct ClassTemplate {
+  Retval memFun();
+  static Retval staticFun();
+};
+
 } // namespace ns
 
 int fun();
@@ -60,6 +66,13 @@
 
   ns::Type::staticFun();
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  ns::ClassTemplate ObjA4;
+  ObjA4.memFun();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  ns::ClassTemplate::staticFun();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
 }
 
 void noWarning() {
@@ -70,13 +83,18 @@
 
   auto R3 = ns::Type::staticFun();
 
+  ns::ClassTemplate ObjB2;
+  auto R4 = ObjB2.memFun();
+
+  auto R5 = ns::ClassTemplate::staticFun();
+
   // test calling a void overload of a checked function
   fun(5);
 
   // test discarding return value of functions that are not configured to be checked
   int I = 1;
   std::launder(&I);
 
-  ns::Type ObjB2;
-  ObjB2.memFun();
+  ns::Type ObjB3;
+  ObjB3.memFun();
 }
Index: docs/clang-tidy/checks/bugprone-unused-return-value.rst
===
--- docs/clang-tidy/checks/bugprone-unused-return-value.rst
+++ docs/clang-tidy/checks/bugprone-unused-return-value.rst
@@ -11,7 +11,7 @@
 .. option:: CheckedFunctions
 
Semicolon-separated list of functions to check. Defaults to
-   ``::std::async;::std::launder;::std::remove;::std::remove_if;::std::unique`

[PATCH] D45891: [clang-tidy] Improve bugprone-unused-return-value check

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

Could someone help getting this merged? I don't have commit access to the repo.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45891



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

[PATCH] D45891: [clang-tidy] Improve bugprone-unused-return-value check

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

Thank you!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45891



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

[PATCH] D46317: [clang-tidy] New check bugprone-map-subscript-operator-lookup

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun created this revision.
khuttun added a reviewer: alexfh.
Herald added subscribers: xazax.hun, mgorny.

Warns on std::map and std::unordered_map lookups done with operator[]. 
Inserting values with operator[] is still allowed.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46317

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/MapSubscriptOperatorLookupCheck.cpp
  clang-tidy/bugprone/MapSubscriptOperatorLookupCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-map-subscript-operator-lookup.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-map-subscript-operator-lookup.cpp

Index: test/clang-tidy/bugprone-map-subscript-operator-lookup.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-map-subscript-operator-lookup.cpp
@@ -0,0 +1,67 @@
+// RUN: %check_clang_tidy %s bugprone-map-subscript-operator-lookup %t
+
+namespace std {
+
+template 
+struct allocator {};
+
+template 
+struct equal_to {};
+
+template 
+struct hash {};
+
+template 
+struct less {};
+
+template 
+struct pair {};
+
+template ,
+  typename Allocator = std::allocator>>
+struct map {
+  T &operator[](const Key &);
+  T &operator[](Key &&);
+};
+
+// the check should be able to match std lib calls even if the functions are
+// declared inside inline namespaces
+inline namespace v1 {
+
+template ,
+  typename KeyEqual = std::equal_to,
+  typename Allocator = std::allocator>>
+struct unordered_map {
+  T &operator[](const Key &);
+  T &operator[](Key &&);
+};
+
+} // namespace v1
+} // namespace std
+
+struct Foo {
+  int getter() const;
+  void setter(int);
+};
+
+void warning() {
+  std::map Map;
+  auto Foo1 = Map[42];
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+
+  std::unordered_map UMap;
+  auto Foo2 = UMap[42];
+  // CHECK-MESSAGES: [[@LINE-1]]:15: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+void noWarning() {
+  std::map MapOk;
+  MapOk[42] = Foo{};
+
+  std::unordered_map UMapOk;
+  UMapOk[42] = Foo{};
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -33,6 +33,7 @@
bugprone-lambda-function-name
bugprone-macro-parentheses
bugprone-macro-repeated-side-effects
+   bugprone-map-subscript-operator-lookup
bugprone-misplaced-operator-in-strlen-in-alloc
bugprone-misplaced-widening-cast
bugprone-move-forwarding-reference
@@ -91,8 +92,8 @@
cppcoreguidelines-pro-type-vararg
cppcoreguidelines-slicing
cppcoreguidelines-special-member-functions
-   fuchsia-header-anon-namespaces (redirects to google-build-namespaces) 
fuchsia-default-arguments
+   fuchsia-header-anon-namespaces (redirects to google-build-namespaces) 
fuchsia-multiple-inheritance
fuchsia-overloaded-operator
fuchsia-statically-constructed-objects
Index: docs/clang-tidy/checks/bugprone-map-subscript-operator-lookup.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/bugprone-map-subscript-operator-lookup.rst
@@ -0,0 +1,6 @@
+.. title:: clang-tidy - bugprone-map-subscript-operator-lookup
+
+bugprone-map-subscript-operator-lookup
+==
+
+FIXME: Describe what patterns does the check detect and why. Give examples.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,11 @@
 Improvements to clang-tidy
 --
 
+- New :doc:`bugprone-map-subscript-operator-lookup
+  ` check
+
+  FIXME: add release notes.
+
 - New module `abseil` for checks related to the `Abseil `_
   library.
 
Index: clang-tidy/bugprone/MapSubscriptOperatorLookupCheck.h
===
--- /dev/null
+++ clang-tidy/bugprone/MapSubscriptOperatorLookupCheck.h
@@ -0,0 +1,35 @@
+//===--- MapSubscriptOperatorLookupCheck.h - clang-tidy--*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MAPSUBSCRIPTOPERATORLOOKUPCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_MAPSUBSCRIPTOPERATORLOOKUPCHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// FIXME: Write a short description.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone

[PATCH] D46317: [clang-tidy] New check bugprone-map-subscript-operator-lookup

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

This checker is inspired by Louis Brandy's excellent CppCon 2017 talk 
(https://www.youtube.com/watch?v=lkgszkPnV8g) where he describes the 
side-effects of std::map::operator[] as being one of the most common source of 
bugs in Facebook's C++ code. In the talk he mentions that banning the usage of 
the operator was discussed at Facebook. This checker ban's reading the map with 
it, writing is still allowed.

The documentation is intentionally still missing from the commit. If the 
reviewers feel that this sort of checker would be useful for clang-tidy, I'll 
add the documentation and more tests.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46317



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

[PATCH] D46317: [clang-tidy] New check bugprone-map-subscript-operator-lookup

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun abandoned this revision.
khuttun added a comment.

Abandoning this. The false positive rate would be too high for this checker.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46317



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

[PATCH] D46317: [clang-tidy] New check bugprone-map-subscript-operator-lookup

[Kalle Huttunen via Phabricator via cfe-commits]

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

In D46317#1988261 , @lebedev.ri wrote:

> IMHO we should proceed with this patch.
>  There's been several patches recently that seem to not care about 
> false-positive rate,
>  and in this case the cost of true-positive can be humongous, as i/we've been 
> finding
>  lots of bugs exactly like this in openmp optimization pass, attributor 
> framework :/


When developing this checker I tested it on LLVM code base, and there was 
hundreds of warnings. Looking at my notes from then, the ones that I analyzed 
fell in to couple of different categories:

- The program logic is such that it's known that the key is found in the map 
(in some of these cases `count()` is asserted before the lookup)
- The code does `find()` vs.` end()` comparison before the lookup

From these the at least the first category could be considered false positives.

I also think that this checker could bring value, though. For example when 
enabled on a new project from the start. I should have time to finish it if 
reviewers feel that we should reconsider it. Also, go ahead and share if you 
have any ideas on how to make the checker more accurate.




Comment at: test/clang-tidy/bugprone-map-subscript-operator-lookup.cpp:60
+}
+
+void noWarning() {

mgehre wrote:
> We often have code like
> ```
> auto &V = Map[Idx];
> if (!V) {
>   V = init();
> }
> use(V);
> ```
> Would that be flagged by this check? Could you add a test for it (possibly 
> with `FIXME`)?
This would not be flagged, as the reference is non-const


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D46317



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

[PATCH] D46317: [clang-tidy] New check bugprone-map-subscript-operator-lookup

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun updated this revision to Diff 259656.
khuttun added a comment.

The patch is now updated to be based on the monorepo. I also fixed all the 
comments from the previous review.


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

https://reviews.llvm.org/D46317

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/MapSubscriptOperatorLookupCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/MapSubscriptOperatorLookupCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-map-subscript-operator-lookup.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup-custom.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup.cpp
@@ -0,0 +1,267 @@
+// RUN: %check_clang_tidy %s bugprone-map-subscript-operator-lookup %t
+
+namespace std {
+
+template 
+struct allocator {};
+
+template 
+struct equal_to {};
+
+template 
+struct hash {};
+
+template 
+struct less {};
+
+template 
+struct pair {};
+
+template ,
+  typename Allocator = std::allocator>>
+struct map {
+  T &operator[](const Key &);
+  T &operator[](Key &&);
+};
+
+// the check should be able to match std lib calls even if the functions are
+// declared inside inline namespaces
+inline namespace v1 {
+
+template ,
+  typename KeyEqual = std::equal_to,
+  typename Allocator = std::allocator>>
+struct unordered_map {
+  T &operator[](const Key &);
+  T &operator[](Key &&);
+};
+
+} // namespace v1
+} // namespace std
+
+struct Foo {
+  int getter() const;
+  void setter(int);
+  bool operator==(const Foo &) const;
+};
+
+using FooPtr = Foo *;
+
+void readInt(const int &);
+void writeInt(int &);
+void copyInt(int);
+
+void readFoo(const Foo &);
+void writeFoo(Foo &);
+void copyFoo(Foo);
+
+void copyFooPtr(Foo *);
+void copyConstFooPtr(const Foo *);
+
+std::map IntMap;
+std::map FooMap;
+std::unordered_map FooPtrMap;
+
+void copyFromMap() {
+  int A = IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  Foo B = FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  FooPtr C = FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+void constRefFromMap() {
+  const int &A = IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:18: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  const Foo &B = FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:18: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  const FooPtr &C = FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+void constPtrFromMap() {
+  const int *A = &IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  const Foo *B = &FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  const FooPtr *C = &FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:22: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+int returnInt() {
+  return IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+Foo returnFoo() {
+  return FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+FooPtr returnFooPtr() {
+  return FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+const int &returnRefToConstInt() {
+  return IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+const Foo &returnRefToConstFoo() {
+  return FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+const FooPtr &returnRefToConstFooPtr() {
+  return FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+con

[PATCH] D46317: [clang-tidy] New check bugprone-map-subscript-operator-lookup

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

I ran the check for LLVM code using configuration `key: 
bugprone-map-subscript-operator-lookup.MapTypes, value: 
"::std::map;::std::unordered_map;::llvm::DenseMapBase;::llvm::MapVector;::llvm::StringMap"`.
 It gave 1212 warnings (see the attachment).F11784386: warnings.txt 



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

https://reviews.llvm.org/D46317



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

[PATCH] D46317: [clang-tidy] New check bugprone-map-subscript-operator-lookup

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun updated this revision to Diff 259843.
khuttun added a comment.

Documented the MapTypes check option.


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

https://reviews.llvm.org/D46317

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/MapSubscriptOperatorLookupCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/MapSubscriptOperatorLookupCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-map-subscript-operator-lookup.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup-custom.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup.cpp
@@ -0,0 +1,267 @@
+// RUN: %check_clang_tidy %s bugprone-map-subscript-operator-lookup %t
+
+namespace std {
+
+template 
+struct allocator {};
+
+template 
+struct equal_to {};
+
+template 
+struct hash {};
+
+template 
+struct less {};
+
+template 
+struct pair {};
+
+template ,
+  typename Allocator = std::allocator>>
+struct map {
+  T &operator[](const Key &);
+  T &operator[](Key &&);
+};
+
+// the check should be able to match std lib calls even if the functions are
+// declared inside inline namespaces
+inline namespace v1 {
+
+template ,
+  typename KeyEqual = std::equal_to,
+  typename Allocator = std::allocator>>
+struct unordered_map {
+  T &operator[](const Key &);
+  T &operator[](Key &&);
+};
+
+} // namespace v1
+} // namespace std
+
+struct Foo {
+  int getter() const;
+  void setter(int);
+  bool operator==(const Foo &) const;
+};
+
+using FooPtr = Foo *;
+
+void readInt(const int &);
+void writeInt(int &);
+void copyInt(int);
+
+void readFoo(const Foo &);
+void writeFoo(Foo &);
+void copyFoo(Foo);
+
+void copyFooPtr(Foo *);
+void copyConstFooPtr(const Foo *);
+
+std::map IntMap;
+std::map FooMap;
+std::unordered_map FooPtrMap;
+
+void copyFromMap() {
+  int A = IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  Foo B = FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  FooPtr C = FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+void constRefFromMap() {
+  const int &A = IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:18: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  const Foo &B = FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:18: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  const FooPtr &C = FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+void constPtrFromMap() {
+  const int *A = &IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  const Foo *B = &FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  const FooPtr *C = &FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:22: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+int returnInt() {
+  return IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+Foo returnFoo() {
+  return FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+FooPtr returnFooPtr() {
+  return FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+const int &returnRefToConstInt() {
+  return IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+const Foo &returnRefToConstFoo() {
+  return FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+const FooPtr &returnRefToConstFooPtr() {
+  return FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+const int *returnPtrToConstInt() {
+  return &IntMap[1];
+  // CHECK-MESSAGE

[PATCH] D46317: [clang-tidy] New check bugprone-map-subscript-operator-lookup

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun updated this revision to Diff 261141.
khuttun added a comment.

Fixed @Eugene.Zelenko's comments


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

https://reviews.llvm.org/D46317

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/MapSubscriptOperatorLookupCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/MapSubscriptOperatorLookupCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-map-subscript-operator-lookup.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup-custom.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup.cpp
@@ -0,0 +1,267 @@
+// RUN: %check_clang_tidy %s bugprone-map-subscript-operator-lookup %t
+
+namespace std {
+
+template 
+struct allocator {};
+
+template 
+struct equal_to {};
+
+template 
+struct hash {};
+
+template 
+struct less {};
+
+template 
+struct pair {};
+
+template ,
+  typename Allocator = std::allocator>>
+struct map {
+  T &operator[](const Key &);
+  T &operator[](Key &&);
+};
+
+// the check should be able to match std lib calls even if the functions are
+// declared inside inline namespaces
+inline namespace v1 {
+
+template ,
+  typename KeyEqual = std::equal_to,
+  typename Allocator = std::allocator>>
+struct unordered_map {
+  T &operator[](const Key &);
+  T &operator[](Key &&);
+};
+
+} // namespace v1
+} // namespace std
+
+struct Foo {
+  int getter() const;
+  void setter(int);
+  bool operator==(const Foo &) const;
+};
+
+using FooPtr = Foo *;
+
+void readInt(const int &);
+void writeInt(int &);
+void copyInt(int);
+
+void readFoo(const Foo &);
+void writeFoo(Foo &);
+void copyFoo(Foo);
+
+void copyFooPtr(Foo *);
+void copyConstFooPtr(const Foo *);
+
+std::map IntMap;
+std::map FooMap;
+std::unordered_map FooPtrMap;
+
+void copyFromMap() {
+  int A = IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  Foo B = FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  FooPtr C = FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+void constRefFromMap() {
+  const int &A = IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:18: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  const Foo &B = FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:18: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  const FooPtr &C = FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+void constPtrFromMap() {
+  const int *A = &IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  const Foo *B = &FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  const FooPtr *C = &FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:22: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+int returnInt() {
+  return IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+Foo returnFoo() {
+  return FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+FooPtr returnFooPtr() {
+  return FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+const int &returnRefToConstInt() {
+  return IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+const Foo &returnRefToConstFoo() {
+  return FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+const FooPtr &returnRefToConstFooPtr() {
+  return FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+const int *returnPtrToConstInt() {
+  return &IntMap[1];
+  // CHECK-MESSAGES: [[

[PATCH] D46317: [clang-tidy] New check bugprone-map-subscript-operator-lookup

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun updated this revision to Diff 261142.
khuttun added a comment.

Accidentally removed the options documentation in the previous patch... Now 
added it back.


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

https://reviews.llvm.org/D46317

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/MapSubscriptOperatorLookupCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/MapSubscriptOperatorLookupCheck.h
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-map-subscript-operator-lookup.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup-custom.cpp
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-map-subscript-operator-lookup.cpp
@@ -0,0 +1,267 @@
+// RUN: %check_clang_tidy %s bugprone-map-subscript-operator-lookup %t
+
+namespace std {
+
+template 
+struct allocator {};
+
+template 
+struct equal_to {};
+
+template 
+struct hash {};
+
+template 
+struct less {};
+
+template 
+struct pair {};
+
+template ,
+  typename Allocator = std::allocator>>
+struct map {
+  T &operator[](const Key &);
+  T &operator[](Key &&);
+};
+
+// the check should be able to match std lib calls even if the functions are
+// declared inside inline namespaces
+inline namespace v1 {
+
+template ,
+  typename KeyEqual = std::equal_to,
+  typename Allocator = std::allocator>>
+struct unordered_map {
+  T &operator[](const Key &);
+  T &operator[](Key &&);
+};
+
+} // namespace v1
+} // namespace std
+
+struct Foo {
+  int getter() const;
+  void setter(int);
+  bool operator==(const Foo &) const;
+};
+
+using FooPtr = Foo *;
+
+void readInt(const int &);
+void writeInt(int &);
+void copyInt(int);
+
+void readFoo(const Foo &);
+void writeFoo(Foo &);
+void copyFoo(Foo);
+
+void copyFooPtr(Foo *);
+void copyConstFooPtr(const Foo *);
+
+std::map IntMap;
+std::map FooMap;
+std::unordered_map FooPtrMap;
+
+void copyFromMap() {
+  int A = IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  Foo B = FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:11: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  FooPtr C = FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:14: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+void constRefFromMap() {
+  const int &A = IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:18: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  const Foo &B = FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:18: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  const FooPtr &C = FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:21: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+void constPtrFromMap() {
+  const int *A = &IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  const Foo *B = &FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:19: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+  const FooPtr *C = &FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:22: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+int returnInt() {
+  return IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+Foo returnFoo() {
+  return FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+FooPtr returnFooPtr() {
+  return FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+const int &returnRefToConstInt() {
+  return IntMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+const Foo &returnRefToConstFoo() {
+  return FooMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+const FooPtr &returnRefToConstFooPtr() {
+  return FooPtrMap[1];
+  // CHECK-MESSAGES: [[@LINE-1]]:10: warning: do not use operator[] for unordered_map lookup [bugprone-map-subscript-operator-lookup]
+}
+
+const int *returnPtrTo

[PATCH] D46317: [clang-tidy] New check bugprone-map-subscript-operator-lookup

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

Any comments on this? Is this checker something that could be part of 
clang-tidy?


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

https://reviews.llvm.org/D46317



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

[PATCH] D46317: [clang-tidy] New check bugprone-map-subscript-operator-lookup

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

In D46317#2027071 , @aaron.ballman 
wrote:

> In D46317#2023406 , @khuttun wrote:
>
> > Any comments on this? Is this checker something that could be part of 
> > clang-tidy?
>
>
> Thank you for posting some of the diagnostics found by the check, that was 
> really helpful information! I spot-checked ~10 of the issues it reported and 
> all of them were false positives. Were you able to find any true positives 
> from that list? I think 1200 reports without any true positives indicates 
> that the check may be too chatty to include (it may also suggest that 
> `bugprone` is the wrong place for the check).


It's difficult to spot actual functionality bugs without knowing the code 
better, but there's plenty of unnecessary double-lookups (count()/find() + 
operator[]) reported, for example:

clang-tools-extra/clang-reorder-fields/ReorderFieldsAction.cpp:75:30
clang-tools-extra/clang-tidy/bugprone/ForwardDeclarationNamespaceCheck.cpp:157:33
clang-tools-extra/clang-tidy/modernize/DeprecatedHeadersCheck.cpp:106:29


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

https://reviews.llvm.org/D46317



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

[PATCH] D41056: [clang-tidy] New check misc-uniqueptr-release-unused-retval

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun created this revision.
Herald added subscribers: xazax.hun, mgorny.

Detects calls to std::unique_ptr::release where the return value is unused.


https://reviews.llvm.org/D41056

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/UniqueptrReleaseUnusedRetvalCheck.cpp
  clang-tidy/misc/UniqueptrReleaseUnusedRetvalCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-uniqueptr-release-unused-retval.rst
  test/clang-tidy/misc-uniqueptr-release-unused-retval.cpp

Index: test/clang-tidy/misc-uniqueptr-release-unused-retval.cpp
===
--- /dev/null
+++ test/clang-tidy/misc-uniqueptr-release-unused-retval.cpp
@@ -0,0 +1,49 @@
+// RUN: %check_clang_tidy %s misc-uniqueptr-release-unused-retval %t
+
+namespace std {
+template 
+struct unique_ptr {
+  T *operator->();
+  T *release();
+};
+} // namespace std
+
+struct Foo {
+  int release();
+};
+
+template 
+void callRelease(T &t) { t.release(); }
+// CHECK-MESSAGES: [[@LINE-1]]:26: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval]
+
+using FooPtr = std::unique_ptr;
+
+template 
+struct Derived : public std::unique_ptr {
+};
+
+void deleteThis(Foo *pointer) { delete pointer; }
+
+void Warning() {
+  std::unique_ptr p1;
+  p1.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval]
+  { p1.release(); }
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval]
+  callRelease(p1);
+  FooPtr fp;
+  fp.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval]
+  Derived dp;
+  dp.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused std::unique_ptr::release return value [misc-uniqueptr-release-unused-retval]
+}
+
+void NoWarning() {
+  std::unique_ptr p2;
+  auto q = p2.release();
+  delete p2.release();
+  deleteThis(p2.release());
+  p2->release();
+  p2.release()->release();
+}
Index: docs/clang-tidy/checks/misc-uniqueptr-release-unused-retval.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/misc-uniqueptr-release-unused-retval.rst
@@ -0,0 +1,45 @@
+.. title:: clang-tidy - misc-uniqueptr-release-unused-retval
+
+misc-uniqueptr-release-unused-retval
+
+
+Warns if the return value of ``std::unique_ptr::release()`` is not used.
+
+Discarding the return value results in leaking the managed object, if the
+pointer isn't stored anywhere else. This can happen for example when
+``release()`` is incorrectly used instead of ``reset()``:
+
+.. code-block:: c++
+
+  void deleteObject() {
+MyUniquePtr.release();
+  }
+
+The check will warn about this. The fix is to replace the ``release()`` call
+with ``reset()``.
+
+Discarding the ``release()`` return value doesn't necessary result in a leak if
+the pointer is also stored somewhere else:
+
+.. code-block:: c++
+
+  void f(std::unique_ptr p) {
+// store the raw pointer
+storePointer(p.get());
+
+// prevent destroying the Foo object when the unique_ptr is destructed
+p.release();
+  }
+
+The check warns also here. Although there's no leak here, the code can still be
+improved by using the ``release()`` return value:
+
+.. code-block:: c++
+
+  void f(std::unique_ptr p) {
+storePointer(p.release());
+  }
+
+This eliminates the possibility that code causing ``f()`` to return, thus
+causing ``p``'s destructor to be called and making the stored raw pointer
+dangle, is added between ``storePointer()`` and ``release()`` calls.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -142,6 +142,7 @@
misc-throw-by-value-catch-by-reference
misc-unconventional-assign-operator
misc-undelegated-constructor
+   misc-uniqueptr-release-unused-retval
misc-uniqueptr-reset-release
misc-unused-alias-decls
misc-unused-parameters
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -57,6 +57,11 @@
 Improvements to clang-tidy
 --
 
+- New `misc-uniqueptr-release-unused-retval
+  `_ check
+
+  Detects calls to std::unique_ptr::release where the return value is unused.
+
 - New module `fuchsia` for Fuchsia style checks.
 
 - New module `objc` for Objective-C style checks.
Index: clang-tidy/misc/UniqueptrReleaseUnusedRetvalCheck.h
===
--- /dev/null
+++ clang-tidy/misc/Uniqueptr

[PATCH] D41056: [clang-tidy] New check misc-uniqueptr-release-unused-retval

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

In https://reviews.llvm.org/D41056#950570, @Eugene.Zelenko wrote:

> May be //bugprone// is better module then //misc//?


Maybe. I can move it if all the reviewers think that it would be better suited 
there.


Repository:
  rL LLVM

https://reviews.llvm.org/D41056



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

[PATCH] D41056: [clang-tidy] New check misc-uniqueptr-release-unused-retval

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

In https://reviews.llvm.org/D41056#951145, @aaron.ballman wrote:

> In https://reviews.llvm.org/D41056#951083, @alexfh wrote:
>
> > In https://reviews.llvm.org/D41056#950605, @khuttun wrote:
> >
> > > In https://reviews.llvm.org/D41056#950570, @Eugene.Zelenko wrote:
> > >
> > > > May be //bugprone// is better module then //misc//?
> > >
> > >
> > > Maybe. I can move it if all the reviewers think that it would be better 
> > > suited there.
> >
> >
> > Yup, bugprone- should be a better category for this, IMO.
> >
> > I wonder whether libc++ folks are interested in marking 
> > unique_ptr::release() with `__attribute__ ((warn_unused_result))`. A 
> > compiler warning (with -Werror) would be a better protection against this 
> > kind of a bug.
>
>
> There's a push in WG21 to mark more of the library with `[[nodiscard]]`: 
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0600r1.pdf
>
> If we have a check for this, I do not think it should be specific to 
> `unique_ptr::release()`, but instead be more broadly applicable to APIs that 
> should be marked `[[nodiscard]]` but are not (currently). P0600R1 is a good 
> place to start, but I'm guessing there are POSIX APIs (among others) that 
> would also qualify.


P0600R1 seems to suggest quite conservative approach (and rightly so, IMO) in 
taking `[[nodiscard]]` in use in std lib. For example it proposes *not* to use 
it with `unique_ptr::release`. I think there could still be room for static 
unused ret val checking for parts of std lib not covered by P0600R1, but also 
for boost, POSIX APIs etc.

What do you think about making this check fully configurable through the check 
options? The options could list the functions to check. The documentation could 
suggest some candidate functions from std lib to configure checks for.


Repository:
  rL LLVM

https://reviews.llvm.org/D41056



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

[PATCH] D45161: [AST] Add new printing policy to suppress printing template arguments

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun created this revision.
khuttun added reviewers: sepavloff, alexfh.
Herald added a subscriber: cfe-commits.

The purpose of this addition is to be able to write AST matchers that match 
class template member functions by fully qualified name, without the need to 
explicitly specify the template arguments in the name.

For example, to match the call to `S::f` here

  template 
  struct S {
void f();
  };
  
  void foo() {
S s;
s.f();
  }

the matcher currently needs to specify the template arguments:

  callExpr(callee(functionDecl(hasName("::S::f"

With the help of this change, it's possible to create a version of `hasName` 
that ignores the template arguments. The matcher could then be written as

  callExpr(callee(functionDecl(hasNameIgnoringTemplateArgs("::S::f"

The motivation for this change is to be able to add checking of class template 
member functions to clang-tidy checker bugprone-unused-return-value: 
http://clang.llvm.org/extra/clang-tidy/checks/bugprone-unused-return-value.html

The discussion about this can be found in the code review for the checker: 
https://reviews.llvm.org/D41655?id=130461#inline-374438


Repository:
  rC Clang

https://reviews.llvm.org/D45161

Files:
  include/clang/AST/PrettyPrinter.h
  lib/AST/TypePrinter.cpp
  unittests/AST/NamedDeclPrinterTest.cpp

Index: unittests/AST/NamedDeclPrinterTest.cpp
===
--- unittests/AST/NamedDeclPrinterTest.cpp
+++ unittests/AST/NamedDeclPrinterTest.cpp
@@ -28,14 +28,19 @@
 
 namespace {
 
+using PrintingPolicyModifier = void (*)(PrintingPolicy &policy);
+
 class PrintMatch : public MatchFinder::MatchCallback {
   SmallString<1024> Printed;
   unsigned NumFoundDecls;
   bool SuppressUnwrittenScope;
+  PrintingPolicyModifier PolicyModifier;
 
 public:
-  explicit PrintMatch(bool suppressUnwrittenScope)
-: NumFoundDecls(0), SuppressUnwrittenScope(suppressUnwrittenScope) {}
+  explicit PrintMatch(bool suppressUnwrittenScope,
+  PrintingPolicyModifier PolicyModifier)
+  : NumFoundDecls(0), SuppressUnwrittenScope(suppressUnwrittenScope),
+PolicyModifier(PolicyModifier) {}
 
   void run(const MatchFinder::MatchResult &Result) override {
 const NamedDecl *ND = Result.Nodes.getNodeAs("id");
@@ -48,6 +53,8 @@
 llvm::raw_svector_ostream Out(Printed);
 PrintingPolicy Policy = Result.Context->getPrintingPolicy();
 Policy.SuppressUnwrittenScope = SuppressUnwrittenScope;
+if (PolicyModifier)
+  PolicyModifier(Policy);
 ND->printQualifiedName(Out, Policy);
   }
 
@@ -64,8 +71,9 @@
 PrintedNamedDeclMatches(StringRef Code, const std::vector &Args,
 bool SuppressUnwrittenScope,
 const DeclarationMatcher &NodeMatch,
-StringRef ExpectedPrinted, StringRef FileName) {
-  PrintMatch Printer(SuppressUnwrittenScope);
+StringRef ExpectedPrinted, StringRef FileName,
+PrintingPolicyModifier PolicyModifier) {
+  PrintMatch Printer(SuppressUnwrittenScope, PolicyModifier);
   MatchFinder Finder;
   Finder.addMatcher(NodeMatch, &Printer);
   std::unique_ptr Factory =
@@ -94,26 +102,30 @@
 
 ::testing::AssertionResult
 PrintedNamedDeclCXX98Matches(StringRef Code, StringRef DeclName,
- StringRef ExpectedPrinted) {
+ StringRef ExpectedPrinted,
+ PrintingPolicyModifier PolicyModifier = nullptr) {
   std::vector Args(1, "-std=c++98");
   return PrintedNamedDeclMatches(Code,
  Args,
  /*SuppressUnwrittenScope*/ false,
  namedDecl(hasName(DeclName)).bind("id"),
  ExpectedPrinted,
- "input.cc");
+ "input.cc",
+ PolicyModifier);
 }
 
 ::testing::AssertionResult
-PrintedWrittenNamedDeclCXX11Matches(StringRef Code, StringRef DeclName,
-StringRef ExpectedPrinted) {
+PrintedWrittenNamedDeclCXX11Matches(
+StringRef Code, StringRef DeclName, StringRef ExpectedPrinted,
+PrintingPolicyModifier PolicyModifier = nullptr) {
   std::vector Args(1, "-std=c++11");
   return PrintedNamedDeclMatches(Code,
  Args,
  /*SuppressUnwrittenScope*/ true,
  namedDecl(hasName(DeclName)).bind("id"),
  ExpectedPrinted,
- "input.cc");
+ "input.cc",
+ PolicyModifier);
 }
 
 } // unnamed namespace
@@ -180,3 +192,20 @@
 "A",
 "X::A"));
 }
+
+TEST(NamedDeclPrinter, TestClassTemplateMemberFunction) {
+  ASSERT_TRUE(PrintedNamedDeclCXX98Matches(
+"template  struct 

[PATCH] D45161: [AST] Add new printing policy to suppress printing template arguments

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun abandoned this revision.
khuttun added a comment.

Figured out a better way to do this by using 
`FunctionDecl::getInstantiatedFromMemberFunction`


Repository:
  rC Clang

https://reviews.llvm.org/D45161



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

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

In https://reviews.llvm.org/D41655#974725, @JonasToth wrote:

> High Integrity C++ has the rule `17.5.1 Do not ignore the result of 
> std::remove, std::remove_if or std::unique`. Do we want to add those to the 
> preconfigured list?


To me these sound like reasonable additions. I can add them and run the checker 
against LLVM source to see if we get any false positives with them.

I also noticed that the checker currently misses unused values inside other 
kinds of statements than compound statement (if statements, case statements 
etc.). I will also update the checker to handle these.




Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:47
+"^::std::launder$|"
+"^::std::unique_ptr<.*>::release$|"
+"^::std::.*::allocate$|"

JonasToth wrote:
> Is the following type a problem for you check?
> 
> `std::unique_ptr>` should not be matchable with regex but I 
> don't know if that would have an impact on the functionality.
`std::unique_ptr>` is also matched. I added a test case for it.



Comment at: test/clang-tidy/bugprone-unused-return-value.cpp:210
+  // test that the check is disabled inside GNU statement expressions
+  ({ std::async(increment, 42); });
+  auto StmtExprRetval = ({ std::async(increment, 42); });

aaron.ballman wrote:
> Not diagnosing this case is questionable -- the return value *is* unused and 
> that's a bad thing. However, this is likely to be an uncommon code pattern, 
> so I'd be fine if you added a FIXME to the AST matcher code that excludes GNU 
> expression statements to handle this case someday, and add a FIXME comment 
> here as well so we know what we would like the behavior to be (you could fix 
> the FIXMEs in a follow-up patch if you'd like).
I'm not so sure whether this code should cause a warning. I see it as 
equivalent to this


```
[]{ return std::async(increment, 42); }();
```

, where the return value of `std::async` is used in the return statement.

One situation related to the GNU statement expressions that the checker 
currently misses is if the return value is unused inside the statement 
expression at some other than the last statement. I can see if this could be 
somehow covered.


https://reviews.llvm.org/D41655



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

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun updated this revision to Diff 130461.
khuttun added a comment.

- Detect unused return values also inside other kinds of statements than 
compound statements
- Ignore void functions in the checker
- Check std::remove, std::remove_if and std::unique by default


https://reviews.llvm.org/D41655

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/UnusedReturnValueCheck.cpp
  clang-tidy/bugprone/UnusedReturnValueCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-unused-return-value.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-unused-return-value-custom.cpp
  test/clang-tidy/bugprone-unused-return-value.cpp

Index: test/clang-tidy/bugprone-unused-return-value.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-unused-return-value.cpp
@@ -0,0 +1,311 @@
+// RUN: %check_clang_tidy %s bugprone-unused-return-value %t
+
+namespace std {
+
+using size_t = decltype(sizeof(int));
+
+using max_align_t = long double;
+
+struct future {};
+
+enum class launch {
+  async,
+  deferred
+};
+
+template 
+future async(Function &&, Args &&...);
+
+template 
+future async(launch, Function &&, Args &&...);
+
+template 
+bool empty(const T &);
+
+template 
+ForwardIt remove(ForwardIt, ForwardIt, const T &);
+
+template 
+ForwardIt remove_if(ForwardIt, ForwardIt, UnaryPredicate);
+
+template 
+ForwardIt unique(ForwardIt, ForwardIt);
+
+// the check should be able to match std lib calls even if the functions are
+// declared inside inline namespaces
+inline namespace v1 {
+
+template 
+T *launder(T *);
+
+} // namespace v1
+
+template 
+struct allocator {
+  using value_type = T;
+  T *allocate(std::size_t);
+  T *allocate(std::size_t, const void *);
+};
+
+template 
+struct allocator_traits {
+  using value_type = typename Alloc::value_type;
+  using pointer = value_type *;
+  using size_type = size_t;
+  using const_void_pointer = const void *;
+  static pointer allocate(Alloc &, size_type);
+  static pointer allocate(Alloc &, size_type, const_void_pointer);
+};
+
+template 
+struct scoped_allocator_adaptor : public OuterAlloc {
+  using pointer = typename allocator_traits::pointer;
+  using size_type = typename allocator_traits::size_type;
+  using const_void_pointer = typename allocator_traits::const_void_pointer;
+  pointer allocate(size_type);
+  pointer allocate(size_type, const_void_pointer);
+};
+
+template 
+struct default_delete {};
+
+template >
+struct unique_ptr {
+  using pointer = T *;
+  pointer release() noexcept;
+};
+
+template >
+struct vector {
+  bool empty() const noexcept;
+};
+
+namespace filesystem {
+
+struct path {
+  bool empty() const noexcept;
+};
+
+} // namespace filesystem
+
+namespace pmr {
+
+struct memory_resource {
+  void *allocate(size_t, size_t = alignof(max_align_t));
+};
+
+template 
+struct polymorphic_allocator {
+  T *allocate(size_t);
+};
+
+} // namespace pmr
+
+} // namespace std
+
+struct Foo {
+  void f();
+};
+
+int increment(int i) {
+  return i + 1;
+}
+
+void useFuture(const std::future &fut);
+
+struct FooAlloc {
+  using value_type = Foo;
+};
+
+void warning() {
+  std::async(increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::async(std::launch::async, increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  Foo F;
+  std::launder(&F);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::unique_ptr UP;
+  UP.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::unique_ptr> UP2;
+  UP2.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::allocator FA;
+  FA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  FA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::allocator_traits>::allocate(FA, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  std::allocator_traits>::allocate(FA, 1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::scoped_allocator_adaptor SAA;
+  SAA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  SAA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning:

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

The checker reports 7 warnings on LLVM + Clang code bases, all on 
std::unique_ptr::release:

lib/Bitcode/Reader/BitReader.cpp:114:3

- release() called on moved-from unique_ptr
- no harm, just unnecessary

lib/ExecutionEngine/ExecutionEngine.cpp:149:7

- release() called before erasing unique_ptr from a container
- false positive?

lib/Target/AMDGPU/GCNIterativeScheduler.cpp:196:5

- release() called before assigning new pointer to unique_ptr
- false positive?

lib/AsmParser/LLParser.cpp:855:3

- get() + release() could be replaced with release()

tools/clang/lib/Lex/ModuleMap.cpp:791:3

- false positive?

tools/clang/tools/extra/clangd/Compiler.cpp:61:3

- get() + release() could potentially be replaced with release()?

unittests/Support/Casting.cpp:144:3

- release() called to avoid calling delete on a pointer to global object
- false positive?


https://reviews.llvm.org/D41655



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

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.
Herald added a subscriber: hintonda.

Any more comments on this?


https://reviews.llvm.org/D41655



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

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

> From what I can tell of these reports, they almost all boil down to ignoring 
> the call to `release()` because the pointer is no longer owned by the 
> `unique_ptr`. This is a pretty reasonable code pattern, but it also seems 
> reasonable to expect users to cast the result to `void` to silence the 
> warning, so I think this is fine. Have you checked any other large C++ code 
> bases, like Qt or boost?

Yep, there's already code also in LLVM where the release() return value is cast 
to void, for example in lib/Bitcode/Reader/BitReader.cpp. I haven't run the 
checker on other large code bases yet, but I can test also that.




Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:45-48
+"^::std::async$|"
+"^::std::launder$|"
+"^::std::remove$|"
+"^::std::remove_if$|"

alexfh wrote:
> alexfh wrote:
> > I strongly suspect that we could get away without regexs here. hasAnyName 
> > supports inline namespaces, so at least the first five entries here are 
> > covered. The main problem with std::unique_ptr<.*>::release is the need to 
> > match any template parameters. I *think*, we could adjust hasName to allow 
> > omitting template parameters (so that `std::unique_ptr::release` would 
> > match specializations of unique_ptr as well). The `empty` and `allocate` 
> > would need some research. Can we just list all specific classes where we 
> > care about `empty`? (Alternatively, can we match `empty` unqualified and 
> > check that it's somewhere inside `std`, but I don't like that much, since 
> > it would add inconsistency in how this check is configured.)
> A clarification: we could go with your current version and optimize this part 
> later. But the option may start being used by someone and then will change - 
> that would be nice to avoid.
> 
> Alternatively, we could switch to hasAnyName right away and leave only the 
> functions that can be easily supported, and then figure out what to do with 
> `release`, `allocate` and `empty`.
> 
> I'd probably prefer the latter.
If the ultimate goal would be to have this checker working without regexes, 
then I agree that we shouldn't introduce a version that uses them in the config 
string, as changing that later would break things.

About creating a version of hasName that ignores template arguments: as I 
understand it we'd need a new PrintingPolicy to suppress printing the template 
argument list in NamedDecl::printQualifiedName. Is this correct?

WG21 P0600R1 lists 8 allocate() and 24 empty() functions in std that should be 
marked with [[nodiscard]]. We could list all of them separately in the checker 
config, but the config string would get quite long. Regex matching handles 
these nicely, but if the performance is unacceptable, we have to look for other 
ways or just skip checking these.


https://reviews.llvm.org/D41655



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