[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Kocsis Ábel via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0f4c70dd3ec6: [clang-tidy] Add spuriously-wake-up-functions 
check (authored by abelkocsis).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70876

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-spuriously-wake-up-functions.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con36-c.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con54-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-spuriously-wake-up-functions.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-spuriously-wake-up-functions.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-spuriously-wake-up-functions.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-spuriously-wake-up-functions.cpp
@@ -0,0 +1,191 @@
+// RUN: %check_clang_tidy %s bugprone-spuriously-wake-up-functions %t -- --
+#define NULL 0
+
+namespace std {
+using intmax_t = int;
+
+template 
+class ratio {
+public:
+  static constexpr intmax_t num = 0;
+  static constexpr intmax_t den = 0;
+  typedef ratio type;
+};
+typedef ratio<1, 1000> milli;
+namespace chrono {
+
+template >
+class duration {
+public:
+  using rep = Rep;
+  using period = Period;
+
+public:
+  constexpr duration() = default;
+  template 
+  constexpr explicit duration(const Rep2 );
+  template 
+  constexpr duration(const duration );
+  ~duration() = default;
+  duration(const duration &) = default;
+};
+
+template 
+class time_point {
+public:
+  using clock = Clock;
+  using duration = Duration;
+
+public:
+  constexpr time_point();
+  constexpr explicit time_point(const duration );
+  template 
+  constexpr time_point(const time_point );
+};
+
+using milliseconds = duration;
+
+class system_clock {
+public:
+  typedef milliseconds duration;
+  typedef duration::rep rep;
+  typedef duration::period period;
+  typedef chrono::time_point time_point;
+
+  static time_point now() noexcept;
+};
+} // namespace chrono
+
+class mutex;
+template 
+class unique_lock {
+public:
+  typedef Mutex mutex_type;
+
+  unique_lock() noexcept;
+  explicit unique_lock(mutex_type );
+};
+
+class mutex {
+public:
+  constexpr mutex() noexcept;
+  ~mutex();
+  mutex(const mutex &) = delete;
+  mutex =(const mutex &) = delete;
+};
+
+enum class cv_status {
+  no_timeout,
+  timeout
+};
+
+class condition_variable {
+public:
+  condition_variable();
+  ~condition_variable();
+  condition_variable(const condition_variable &) = delete;
+
+  void wait(unique_lock );
+  template 
+  void wait(unique_lock , Predicate pred);
+  template 
+  cv_status wait_until(unique_lock ,
+   const chrono::time_point _time){};
+  template 
+  bool wait_until(unique_lock ,
+  const chrono::time_point _time,
+  Predicate pred){};
+  template 
+  cv_status wait_for(unique_lock ,
+ const chrono::duration _time){};
+  template 
+  bool wait_for(unique_lock ,
+const chrono::duration _time,
+Predicate pred){};
+};
+
+} // namespace std
+
+struct Node1 {
+  void *Node1;
+  struct Node1 *next;
+};
+
+static Node1 list;
+static std::mutex m;
+static std::condition_variable condition;
+
+void consume_list_element(std::condition_variable ) {
+  std::unique_lock lk(m);
+
+  if (list.next == nullptr) {
+condition.wait(lk);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait' should be placed inside a while statement or used with a conditional parameter [bugprone-spuriously-wake-up-functions]
+  }
+
+  while (list.next == nullptr) {
+condition.wait(lk);
+  }
+
+  do {
+condition.wait(lk);
+  } while (list.next == nullptr);
+
+  for (;; list.next == nullptr) {
+condition.wait(lk);
+  }
+
+  if (list.next == nullptr) {
+while (list.next == nullptr) {
+  condition.wait(lk);
+}
+  }
+
+  if (list.next == nullptr) {
+do {
+  condition.wait(lk);
+} while (list.next == nullptr);
+  }
+
+  if (list.next == nullptr) {
+for (;; list.next == nullptr) {
+  condition.wait(lk);
+}
+  }
+  using durtype = std::chrono::duration;
+  durtype dur = std::chrono::duration();
+  if (list.next == nullptr) {
+condition.wait_for(lk, dur);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait_for' should be placed inside a while statement or used with a conditional parameter 

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Kocsis Ábel via Phabricator via cfe-commits]

abelkocsis updated this revision to Diff 251829.
abelkocsis added a comment.

Format fix


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-spuriously-wake-up-functions.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con36-c.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con54-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-spuriously-wake-up-functions.c
  
clang-tools-extra/test/clang-tidy/checkers/bugprone-spuriously-wake-up-functions.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone-spuriously-wake-up-functions.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone-spuriously-wake-up-functions.cpp
@@ -0,0 +1,191 @@
+// RUN: %check_clang_tidy %s bugprone-spuriously-wake-up-functions %t -- --
+#define NULL 0
+
+namespace std {
+using intmax_t = int;
+
+template 
+class ratio {
+public:
+  static constexpr intmax_t num = 0;
+  static constexpr intmax_t den = 0;
+  typedef ratio type;
+};
+typedef ratio<1, 1000> milli;
+namespace chrono {
+
+template >
+class duration {
+public:
+  using rep = Rep;
+  using period = Period;
+
+public:
+  constexpr duration() = default;
+  template 
+  constexpr explicit duration(const Rep2 );
+  template 
+  constexpr duration(const duration );
+  ~duration() = default;
+  duration(const duration &) = default;
+};
+
+template 
+class time_point {
+public:
+  using clock = Clock;
+  using duration = Duration;
+
+public:
+  constexpr time_point();
+  constexpr explicit time_point(const duration );
+  template 
+  constexpr time_point(const time_point );
+};
+
+using milliseconds = duration;
+
+class system_clock {
+public:
+  typedef milliseconds duration;
+  typedef duration::rep rep;
+  typedef duration::period period;
+  typedef chrono::time_point time_point;
+
+  static time_point now() noexcept;
+};
+} // namespace chrono
+
+class mutex;
+template 
+class unique_lock {
+public:
+  typedef Mutex mutex_type;
+
+  unique_lock() noexcept;
+  explicit unique_lock(mutex_type );
+};
+
+class mutex {
+public:
+  constexpr mutex() noexcept;
+  ~mutex();
+  mutex(const mutex &) = delete;
+  mutex =(const mutex &) = delete;
+};
+
+enum class cv_status {
+  no_timeout,
+  timeout
+};
+
+class condition_variable {
+public:
+  condition_variable();
+  ~condition_variable();
+  condition_variable(const condition_variable &) = delete;
+
+  void wait(unique_lock );
+  template 
+  void wait(unique_lock , Predicate pred);
+  template 
+  cv_status wait_until(unique_lock ,
+   const chrono::time_point _time){};
+  template 
+  bool wait_until(unique_lock ,
+  const chrono::time_point _time,
+  Predicate pred){};
+  template 
+  cv_status wait_for(unique_lock ,
+ const chrono::duration _time){};
+  template 
+  bool wait_for(unique_lock ,
+const chrono::duration _time,
+Predicate pred){};
+};
+
+} // namespace std
+
+struct Node1 {
+  void *Node1;
+  struct Node1 *next;
+};
+
+static Node1 list;
+static std::mutex m;
+static std::condition_variable condition;
+
+void consume_list_element(std::condition_variable ) {
+  std::unique_lock lk(m);
+
+  if (list.next == nullptr) {
+condition.wait(lk);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait' should be placed inside a while statement or used with a conditional parameter [bugprone-spuriously-wake-up-functions]
+  }
+
+  while (list.next == nullptr) {
+condition.wait(lk);
+  }
+
+  do {
+condition.wait(lk);
+  } while (list.next == nullptr);
+
+  for (;; list.next == nullptr) {
+condition.wait(lk);
+  }
+
+  if (list.next == nullptr) {
+while (list.next == nullptr) {
+  condition.wait(lk);
+}
+  }
+
+  if (list.next == nullptr) {
+do {
+  condition.wait(lk);
+} while (list.next == nullptr);
+  }
+
+  if (list.next == nullptr) {
+for (;; list.next == nullptr) {
+  condition.wait(lk);
+}
+  }
+  using durtype = std::chrono::duration;
+  durtype dur = std::chrono::duration();
+  if (list.next == nullptr) {
+condition.wait_for(lk, dur);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait_for' should be placed inside a while statement or used with a conditional parameter [bugprone-spuriously-wake-up-functions]
+  }
+  if (list.next == nullptr) {
+condition.wait_for(lk, dur, [] { return 1; });
+  }
+ 

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM, do you need me to commit on your behalf?




Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.c:165
+int main() { return 0; }
\ No newline at end of file


Please add a newline to the end of the file.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876



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

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Kocsis Ábel via Phabricator via cfe-commits]

abelkocsis updated this revision to Diff 245113.
abelkocsis added a comment.

Test cases adding, checker modifying to pass new cases.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-spuriously-wake-up-functions.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con36-c.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con54-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.c
  clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp
@@ -0,0 +1,191 @@
+// RUN: %check_clang_tidy %s bugprone-spuriously-wake-up-functions %t -- --
+#define NULL 0
+
+namespace std {
+using intmax_t = int;
+
+template 
+class ratio {
+public:
+  static constexpr intmax_t num = 0;
+  static constexpr intmax_t den = 0;
+  typedef ratio type;
+};
+typedef ratio<1, 1000> milli;
+namespace chrono {
+
+template >
+class duration {
+public:
+  using rep = Rep;
+  using period = Period;
+
+public:
+  constexpr duration() = default;
+  template 
+  constexpr explicit duration(const Rep2 );
+  template 
+  constexpr duration(const duration );
+  ~duration() = default;
+  duration(const duration &) = default;
+};
+
+template 
+class time_point {
+public:
+  using clock = Clock;
+  using duration = Duration;
+
+public:
+  constexpr time_point();
+  constexpr explicit time_point(const duration );
+  template 
+  constexpr time_point(const time_point );
+};
+
+using milliseconds = duration;
+
+class system_clock {
+public:
+  typedef milliseconds duration;
+  typedef duration::rep rep;
+  typedef duration::period period;
+  typedef chrono::time_point time_point;
+
+  static time_point now() noexcept;
+};
+} // namespace chrono
+
+class mutex;
+template 
+class unique_lock {
+public:
+  typedef Mutex mutex_type;
+
+  unique_lock() noexcept;
+  explicit unique_lock(mutex_type );
+};
+
+class mutex {
+public:
+  constexpr mutex() noexcept;
+  ~mutex();
+  mutex(const mutex &) = delete;
+  mutex =(const mutex &) = delete;
+};
+
+enum class cv_status {
+  no_timeout,
+  timeout
+};
+
+class condition_variable {
+public:
+  condition_variable();
+  ~condition_variable();
+  condition_variable(const condition_variable &) = delete;
+
+  void wait(unique_lock );
+  template 
+  void wait(unique_lock , Predicate pred);
+  template 
+  cv_status wait_until(unique_lock ,
+   const chrono::time_point _time){};
+  template 
+  bool wait_until(unique_lock ,
+  const chrono::time_point _time,
+  Predicate pred){};
+  template 
+  cv_status wait_for(unique_lock ,
+ const chrono::duration _time){};
+  template 
+  bool wait_for(unique_lock ,
+const chrono::duration _time,
+Predicate pred){};
+};
+
+} // namespace std
+
+struct Node1 {
+  void *Node1;
+  struct Node1 *next;
+};
+
+static Node1 list;
+static std::mutex m;
+static std::condition_variable condition;
+
+void consume_list_element(std::condition_variable ) {
+  std::unique_lock lk(m);
+
+  if (list.next == nullptr) {
+condition.wait(lk);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait' should be placed inside a while statement or used with a conditional parameter [bugprone-spuriously-wake-up-functions]
+  }
+
+  while (list.next == nullptr) {
+condition.wait(lk);
+  }
+
+  do {
+condition.wait(lk);
+  } while (list.next == nullptr);
+
+  for (;; list.next == nullptr) {
+condition.wait(lk);
+  }
+
+  if (list.next == nullptr) {
+while (list.next == nullptr) {
+  condition.wait(lk);
+}
+  }
+
+  if (list.next == nullptr) {
+do {
+  condition.wait(lk);
+} while (list.next == nullptr);
+  }
+
+  if (list.next == nullptr) {
+for (;; list.next == nullptr) {
+  condition.wait(lk);
+}
+  }
+  using durtype = std::chrono::duration;
+  durtype dur = std::chrono::duration();
+  if (list.next == nullptr) {
+condition.wait_for(lk, dur);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait_for' should be placed inside a while statement or used with a conditional parameter [bugprone-spuriously-wake-up-functions]
+  }
+  if (list.next == nullptr) {
+condition.wait_for(lk, dur, [] { return 1; });

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Aaron Ballman via Phabricator via cfe-commits]

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

LGTM aside from a test coverage request.




Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.c:26-30
+  if (list_c.next == NULL) {
+if (0 != cnd_wait(_c, )) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 'cnd_wait' should be placed 
inside a while statement [bugprone-spuriously-wake-up-functions]
+}
+  }

Can you add a test case to demonstrate the behavior when there's not a compound 
statement involved? And another one for a slightly more complex if predicate? 
e.g.,
```
if (list_c.next == NULL)
  if (0 != cnd_wait(_c, ))
;

if (list_c.next == NULL && 0 != cnd_wait(_c, ))
  ;
```
Both of these should be flagged similar to the form with the compound statement.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876



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

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Kocsis Ábel via Phabricator via cfe-commits]

abelkocsis updated this revision to Diff 244098.
abelkocsis added a comment.

docs/list.rst update


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-spuriously-wake-up-functions.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con36-c.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con54-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.c
  clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp
@@ -0,0 +1,191 @@
+// RUN: %check_clang_tidy %s bugprone-spuriously-wake-up-functions %t -- --
+#define NULL 0
+
+namespace std {
+using intmax_t = int;
+
+template 
+class ratio {
+public:
+  static constexpr intmax_t num = 0;
+  static constexpr intmax_t den = 0;
+  typedef ratio type;
+};
+typedef ratio<1, 1000> milli;
+namespace chrono {
+
+template >
+class duration {
+public:
+  using rep = Rep;
+  using period = Period;
+
+public:
+  constexpr duration() = default;
+  template 
+  constexpr explicit duration(const Rep2 );
+  template 
+  constexpr duration(const duration );
+  ~duration() = default;
+  duration(const duration &) = default;
+};
+
+template 
+class time_point {
+public:
+  using clock = Clock;
+  using duration = Duration;
+
+public:
+  constexpr time_point();
+  constexpr explicit time_point(const duration );
+  template 
+  constexpr time_point(const time_point );
+};
+
+using milliseconds = duration;
+
+class system_clock {
+public:
+  typedef milliseconds duration;
+  typedef duration::rep rep;
+  typedef duration::period period;
+  typedef chrono::time_point time_point;
+
+  static time_point now() noexcept;
+};
+} // namespace chrono
+
+class mutex;
+template 
+class unique_lock {
+public:
+  typedef Mutex mutex_type;
+
+  unique_lock() noexcept;
+  explicit unique_lock(mutex_type );
+};
+
+class mutex {
+public:
+  constexpr mutex() noexcept;
+  ~mutex();
+  mutex(const mutex &) = delete;
+  mutex =(const mutex &) = delete;
+};
+
+enum class cv_status {
+  no_timeout,
+  timeout
+};
+
+class condition_variable {
+public:
+  condition_variable();
+  ~condition_variable();
+  condition_variable(const condition_variable &) = delete;
+
+  void wait(unique_lock );
+  template 
+  void wait(unique_lock , Predicate pred);
+  template 
+  cv_status wait_until(unique_lock ,
+   const chrono::time_point _time){};
+  template 
+  bool wait_until(unique_lock ,
+  const chrono::time_point _time,
+  Predicate pred){};
+  template 
+  cv_status wait_for(unique_lock ,
+ const chrono::duration _time){};
+  template 
+  bool wait_for(unique_lock ,
+const chrono::duration _time,
+Predicate pred){};
+};
+
+} // namespace std
+
+struct Node1 {
+  void *Node1;
+  struct Node1 *next;
+};
+
+static Node1 list;
+static std::mutex m;
+static std::condition_variable condition;
+
+void consume_list_element(std::condition_variable ) {
+  std::unique_lock lk(m);
+
+  if (list.next == nullptr) {
+condition.wait(lk);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait' should be placed inside a while statement or used with a conditional parameter [bugprone-spuriously-wake-up-functions]
+  }
+
+  while (list.next == nullptr) {
+condition.wait(lk);
+  }
+
+  do {
+condition.wait(lk);
+  } while (list.next == nullptr);
+
+  for (;; list.next == nullptr) {
+condition.wait(lk);
+  }
+
+  if (list.next == nullptr) {
+while (list.next == nullptr) {
+  condition.wait(lk);
+}
+  }
+
+  if (list.next == nullptr) {
+do {
+  condition.wait(lk);
+} while (list.next == nullptr);
+  }
+
+  if (list.next == nullptr) {
+for (;; list.next == nullptr) {
+  condition.wait(lk);
+}
+  }
+  using durtype = std::chrono::duration;
+  durtype dur = std::chrono::duration();
+  if (list.next == nullptr) {
+condition.wait_for(lk, dur);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait_for' should be placed inside a while statement or used with a conditional parameter [bugprone-spuriously-wake-up-functions]
+  }
+  if (list.next == nullptr) {
+condition.wait_for(lk, dur, [] { return 1; });
+  }
+  while (list.next == 

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Balázs Benics via Phabricator via cfe-commits]

steakhal added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:300-301
 
+   `cert-con36-c `_, `bugprone-spuriously-wake-up-functions 
`_, "Yes"
+   `cert-con54-cpp `_, 
`bugprone-spuriously-wake-up-functions 
`_, "Yes"
`cert-dcl03-c `_, `misc-static-assert 
`_, "Yes"

In case the checker does not offer automatic fixes, remove `"Yes"` from the 
given table entries.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876



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

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Kocsis Ábel via Phabricator via cfe-commits]

abelkocsis updated this revision to Diff 243779.
abelkocsis added a comment.

Checker update to analyse language, separated checks for C and C++.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-spuriously-wake-up-functions.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con36-c.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con54-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.c
  clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp
@@ -0,0 +1,191 @@
+// RUN: %check_clang_tidy %s bugprone-spuriously-wake-up-functions %t -- --
+#define NULL 0
+
+namespace std {
+using intmax_t = int;
+
+template 
+class ratio {
+public:
+  static constexpr intmax_t num = 0;
+  static constexpr intmax_t den = 0;
+  typedef ratio type;
+};
+typedef ratio<1, 1000> milli;
+namespace chrono {
+
+template >
+class duration {
+public:
+  using rep = Rep;
+  using period = Period;
+
+public:
+  constexpr duration() = default;
+  template 
+  constexpr explicit duration(const Rep2 );
+  template 
+  constexpr duration(const duration );
+  ~duration() = default;
+  duration(const duration &) = default;
+};
+
+template 
+class time_point {
+public:
+  using clock = Clock;
+  using duration = Duration;
+
+public:
+  constexpr time_point();
+  constexpr explicit time_point(const duration );
+  template 
+  constexpr time_point(const time_point );
+};
+
+using milliseconds = duration;
+
+class system_clock {
+public:
+  typedef milliseconds duration;
+  typedef duration::rep rep;
+  typedef duration::period period;
+  typedef chrono::time_point time_point;
+
+  static time_point now() noexcept;
+};
+} // namespace chrono
+
+class mutex;
+template 
+class unique_lock {
+public:
+  typedef Mutex mutex_type;
+
+  unique_lock() noexcept;
+  explicit unique_lock(mutex_type );
+};
+
+class mutex {
+public:
+  constexpr mutex() noexcept;
+  ~mutex();
+  mutex(const mutex &) = delete;
+  mutex =(const mutex &) = delete;
+};
+
+enum class cv_status {
+  no_timeout,
+  timeout
+};
+
+class condition_variable {
+public:
+  condition_variable();
+  ~condition_variable();
+  condition_variable(const condition_variable &) = delete;
+
+  void wait(unique_lock );
+  template 
+  void wait(unique_lock , Predicate pred);
+  template 
+  cv_status wait_until(unique_lock ,
+   const chrono::time_point _time){};
+  template 
+  bool wait_until(unique_lock ,
+  const chrono::time_point _time,
+  Predicate pred){};
+  template 
+  cv_status wait_for(unique_lock ,
+ const chrono::duration _time){};
+  template 
+  bool wait_for(unique_lock ,
+const chrono::duration _time,
+Predicate pred){};
+};
+
+} // namespace std
+
+struct Node1 {
+  void *Node1;
+  struct Node1 *next;
+};
+
+static Node1 list;
+static std::mutex m;
+static std::condition_variable condition;
+
+void consume_list_element(std::condition_variable ) {
+  std::unique_lock lk(m);
+
+  if (list.next == nullptr) {
+condition.wait(lk);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait' should be placed inside a while statement or used with a conditional parameter [bugprone-spuriously-wake-up-functions]
+  }
+
+  while (list.next == nullptr) {
+condition.wait(lk);
+  }
+
+  do {
+condition.wait(lk);
+  } while (list.next == nullptr);
+
+  for (;; list.next == nullptr) {
+condition.wait(lk);
+  }
+
+  if (list.next == nullptr) {
+while (list.next == nullptr) {
+  condition.wait(lk);
+}
+  }
+
+  if (list.next == nullptr) {
+do {
+  condition.wait(lk);
+} while (list.next == nullptr);
+  }
+
+  if (list.next == nullptr) {
+for (;; list.next == nullptr) {
+  condition.wait(lk);
+}
+  }
+  using durtype = std::chrono::duration;
+  durtype dur = std::chrono::duration();
+  if (list.next == nullptr) {
+condition.wait_for(lk, dur);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait_for' should be placed inside a while statement or used with a conditional parameter [bugprone-spuriously-wake-up-functions]
+  }
+  if (list.next == nullptr) {
+condition.wait_for(lk, dur, [] { 

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.cpp:60
+  .bind("wait"));
+  Finder->addMatcher(
+  ifStmt(anyOf(

Sorry for not noticing this earlier, but I think you can make two calls to 
`addMatcher()`, one for the C check and one for the C++ check. Then you can 
register only the matcher needed for the language mode you are currently in. 
e.g.,
```
if (getLangOpts().CPlusPlus)
  // Check for CON54-CPP
else
  // Check for CON36-C
```
This should make the check slightly more efficient because of the mutually 
exclusive nature of these APIs without changing the behavior.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:75
`bugprone-sizeof-expression `_, , "high"
+   `bugprone-spuriously-wake-up-functions 
`_, , ""
`bugprone-string-constructor `_, "Yes", 
"high"

sylvestre.ledru wrote:
> Could you try to evaluate the severity? :)
> thanks
> 
Given that these are essentially CERT rules, I'd go with the CERT severity for 
them -- however, I say that only because I'm not certain what approach is 
supposed to be taken for these severity markings.

CERT marks these two rules as having a low severity, FWIW.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876



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

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Kocsis Ábel via Phabricator via cfe-commits]

abelkocsis updated this revision to Diff 235469.
abelkocsis marked an inline comment as done.
abelkocsis added a comment.

Checker fixes, updates test file adding test cases for `doWhile` and `for` 
statements, test headers deleted.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-spuriously-wake-up-functions.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con36-c.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con54-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp
@@ -0,0 +1,270 @@
+// RUN: %check_clang_tidy %s bugprone-spuriously-wake-up-functions %t -- --
+#define NULL 0
+
+namespace std {
+using intmax_t = int;
+
+template 
+class ratio {
+public:
+  static constexpr intmax_t num = 0;
+  static constexpr intmax_t den = 0;
+  typedef ratio type;
+};
+typedef ratio<1, 1000> milli;
+namespace chrono {
+
+template >
+class duration {
+public:
+  using rep = Rep;
+  using period = Period;
+
+public:
+  constexpr duration() = default;
+  template 
+  constexpr explicit duration(const Rep2 );
+  template 
+  constexpr duration(const duration );
+  ~duration() = default;
+  duration(const duration &) = default;
+};
+
+template 
+class time_point {
+public:
+  using clock = Clock;
+  using duration = Duration;
+
+public:
+  constexpr time_point();
+  constexpr explicit time_point(const duration );
+  template 
+  constexpr time_point(const time_point );
+};
+
+using milliseconds = duration;
+
+class system_clock {
+public:
+  typedef milliseconds duration;
+  typedef duration::rep rep;
+  typedef duration::period period;
+  typedef chrono::time_point time_point;
+
+  static time_point now() noexcept;
+};
+} // namespace chrono
+
+class mutex;
+template 
+class unique_lock {
+public:
+  typedef Mutex mutex_type;
+
+  unique_lock() noexcept;
+  explicit unique_lock(mutex_type );
+};
+
+class mutex {
+public:
+  constexpr mutex() noexcept;
+  ~mutex();
+  mutex(const mutex &) = delete;
+  mutex =(const mutex &) = delete;
+};
+
+enum class cv_status {
+  no_timeout,
+  timeout
+};
+
+class condition_variable {
+public:
+  condition_variable();
+  ~condition_variable();
+  condition_variable(const condition_variable &) = delete;
+
+  void wait(unique_lock );
+  template 
+  void wait(unique_lock , Predicate pred);
+  template 
+  cv_status wait_until(unique_lock ,
+   const chrono::time_point _time){};
+  template 
+  bool wait_until(unique_lock ,
+  const chrono::time_point _time,
+  Predicate pred){};
+  template 
+  cv_status wait_for(unique_lock ,
+ const chrono::duration _time){};
+  template 
+  bool wait_for(unique_lock ,
+const chrono::duration _time,
+Predicate pred){};
+};
+
+} // namespace std
+
+struct Node1 {
+  void *Node1;
+  struct Node1 *next;
+};
+
+static Node1 list;
+static std::mutex m;
+static std::condition_variable condition;
+
+void consume_list_element(std::condition_variable ) {
+  std::unique_lock lk(m);
+
+  if (list.next == nullptr) {
+condition.wait(lk);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait' should be placed inside a while statement or used with a conditional parameter [bugprone-spuriously-wake-up-functions]
+  }
+
+  while (list.next == nullptr) {
+condition.wait(lk);
+  }
+
+  do {
+condition.wait(lk);
+  } while (list.next == nullptr);
+
+  for (;; list.next == nullptr) {
+condition.wait(lk);
+  }
+
+  if (list.next == nullptr) {
+while (list.next == nullptr) {
+  condition.wait(lk);
+}
+  }
+
+  if (list.next == nullptr) {
+do {
+  condition.wait(lk);
+} while (list.next == nullptr);
+  }
+
+  if (list.next == nullptr) {
+for (;; list.next == nullptr) {
+  condition.wait(lk);
+}
+  }
+  using durtype = std::chrono::duration;
+  durtype dur = std::chrono::duration();
+  if (list.next == nullptr) {
+condition.wait_for(lk, dur);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait_for' should be placed inside a while statement or used with a conditional parameter [bugprone-spuriously-wake-up-functions]
+  }
+  if (list.next == nullptr) {
+condition.wait_for(lk, 

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Kocsis Ábel via Phabricator via cfe-commits]

abelkocsis marked 11 inline comments as done.
abelkocsis added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp:2
+// RUN: %check_clang_tidy %s bugprone-spuriously-wake-up-functions %t -- -- -I 
%S/Inputs/bugprone-spuriously-wake-up-functions/
+#include "condition_variable.h"
+#define NULL 0

aaron.ballman wrote:
> Are you planning to use these declarations in multiple test files? If not, 
> then we typically put all of the header code into the test file rather than 
> added as explicit files.
I'm not sure yet, so I move it into the test file.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876



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

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.cpp:22
+  auto hasUniqueLock = hasDescendant(declRefExpr(hasDeclaration(
+  varDecl(hasType(asString("std::unique_lock"));
+

These should all use `::std::whatever` to guard against pathological code that 
uses `std` inside of another namespace.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.cpp:80
+Str << " or used with a condition parameter";
+  diag(MatchedWait->getExprLoc(), Str.str().str()) << waitName;
+}

This should be simplified into:
```
const auto *MatchedWait = Result.Nodes.getNodeAs("wait");
StringRef WaitName = MatchedWait->getDirectCallee()->getName();
diag(MatchedWait->getExprLoc(), "'%0' should be placed inside a while statement 
%select{|or used with a conditional parameter}1") << WaitName << (WaitName != 
"cnd_wait" && WaitName != cnd_timedwait);
```



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-spuriously-wake-up-functions.rst:6
+
+Finds ``cnd_wait``, ``wait``, ``wait_for``, or ``wait_until`` function calls
+when the function is not invoked from a loop that checks whether a condition

Missing `cnd_timedwait`



Comment at: 
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/chrono.h:49
+} // namespace std
\ No newline at end of file


Please add a newline to the end of the file.



Comment at: 
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/condition_variable.h:41
+} // namespace std
\ No newline at end of file


Please add a newline to the end of the file.



Comment at: 
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/cstdint.h:4
+}
\ No newline at end of file


Please add a newline to the end of the file.



Comment at: 
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/mutex.h:20
+} // namespace std
\ No newline at end of file


Please add a newline to the end of the file.



Comment at: 
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/ratio.h:29
+} // namespace std
\ No newline at end of file


Please add a newline to the end of the file.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp:2
+// RUN: %check_clang_tidy %s bugprone-spuriously-wake-up-functions %t -- -- -I 
%S/Inputs/bugprone-spuriously-wake-up-functions/
+#include "condition_variable.h"
+#define NULL 0

Are you planning to use these declarations in multiple test files? If not, then 
we typically put all of the header code into the test file rather than added as 
explicit files.



Comment at: 
clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp:99
+  }
+}

You are missing test cases that use other kinds of loops, like a `do..while` or 
`for` loop.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876



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

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added inline comments.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:75
`bugprone-sizeof-expression `_, , "high"
+   `bugprone-spuriously-wake-up-functions 
`_, , ""
`bugprone-string-constructor `_, "Yes", 
"high"

Could you try to evaluate the severity? :)
thanks



Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876



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

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Kocsis Ábel via Phabricator via cfe-commits]

abelkocsis updated this revision to Diff 235232.
abelkocsis added a comment.

`docs/clang-tidy/checks/lsit.rst` update


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-spuriously-wake-up-functions.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con36-c.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con54-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/chrono.h
  
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/condition_variable.h
  
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/cstdint.h
  
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/mutex.h
  
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/ratio.h
  clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp
@@ -0,0 +1,99 @@
+// RUN: %check_clang_tidy %s bugprone-spuriously-wake-up-functions %t -- -- -I %S/Inputs/bugprone-spuriously-wake-up-functions/
+#include "condition_variable.h"
+#define NULL 0
+
+struct Node1 {
+  void *Node1;
+  struct Node1 *next;
+};
+
+static Node1 list;
+static std::mutex m;
+static std::condition_variable condition;
+
+void consume_list_element(std::condition_variable ) {
+  std::unique_lock lk(m);
+
+  if (list.next == nullptr) {
+condition.wait(lk);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait' should be placed inside a while statement or used with a condition parameter [bugprone-spuriously-wake-up-functions]
+  }
+
+  while (list.next == nullptr) {
+condition.wait(lk);
+  }
+
+  if (list.next == nullptr) {
+while (list.next == nullptr) {
+  condition.wait(lk);
+}
+  }
+  using durtype = std::chrono::duration;
+  durtype dur = std::chrono::duration();
+  if (list.next == nullptr) {
+condition.wait_for(lk, dur);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait_for' should be placed inside a while statement or used with a condition parameter [bugprone-spuriously-wake-up-functions]
+  }
+  if (list.next == nullptr) {
+condition.wait_for(lk, dur, [] { return 1; });
+  }
+  auto now = std::chrono::system_clock::now();
+  if (list.next == nullptr) {
+condition.wait_until(lk, now);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait_until' should be placed inside a while statement or used with a condition parameter [bugprone-spuriously-wake-up-functions]
+  }
+  if (list.next == nullptr) {
+condition.wait_until(lk, now, [] { return 1; });
+  }
+}
+
+typedef struct mtx_t {
+} mtx_t;
+typedef struct cnd_t {
+} cnd_t;
+struct timespec {};
+
+int cnd_wait(cnd_t *cond, mtx_t *mutex){};
+int cnd_timedwait(cnd_t *cond, mtx_t *mutex,
+  const struct timespec *time_point){};
+
+struct Node1 list_c;
+static mtx_t lock;
+static cnd_t condition_c;
+struct timespec ts;
+
+void consume_list_element(void) {
+
+  if (list_c.next == NULL) {
+if (0 != cnd_wait(_c, )) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 'cnd_wait' should be placed inside a while statement [bugprone-spuriously-wake-up-functions]
+}
+  }
+  while (list_c.next == NULL) {
+if (0 != cnd_wait(_c, )) {
+}
+  }
+  if (list_c.next == NULL) {
+cnd_wait(_c, );
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'cnd_wait' should be placed inside a while statement [bugprone-spuriously-wake-up-functions]
+  }
+  while (list.next == NULL) {
+cnd_wait(_c, );
+  }
+  if (list_c.next == NULL) {
+if (0 != cnd_timedwait(_c, , )) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 'cnd_timedwait' should be placed inside a while statement [bugprone-spuriously-wake-up-functions]
+}
+  }
+  while (list_c.next == NULL) {
+if (0 != cnd_timedwait(_c, , )) {
+}
+  }
+  if (list_c.next == NULL) {
+cnd_timedwait(_c, , );
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'cnd_timedwait' should be placed inside a while statement [bugprone-spuriously-wake-up-functions]
+  }
+  while (list.next == NULL) {
+cnd_timedwait(_c, , );
+  }
+}
Index: clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/ratio.h

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

The `checks.rst` has recently been updated to show "normal" (main entry) checks 
and "aliases" in different tables in D36051 . 
Please register the alias-ness accordingly.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876



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

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Kocsis Ábel via Phabricator via cfe-commits]

abelkocsis updated this revision to Diff 235212.
abelkocsis added a comment.

Adding matcher for `cnd_timedwait` and test cases.
Small fix in documentation


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-spuriously-wake-up-functions.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con36-c.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con54-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/chrono.h
  
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/condition_variable.h
  
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/cstdint.h
  
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/mutex.h
  
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/ratio.h
  clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp
@@ -0,0 +1,99 @@
+// RUN: %check_clang_tidy %s bugprone-spuriously-wake-up-functions %t -- -- -I %S/Inputs/bugprone-spuriously-wake-up-functions/
+#include "condition_variable.h"
+#define NULL 0
+
+struct Node1 {
+  void *Node1;
+  struct Node1 *next;
+};
+
+static Node1 list;
+static std::mutex m;
+static std::condition_variable condition;
+
+void consume_list_element(std::condition_variable ) {
+  std::unique_lock lk(m);
+
+  if (list.next == nullptr) {
+condition.wait(lk);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait' should be placed inside a while statement or used with a condition parameter [bugprone-spuriously-wake-up-functions]
+  }
+
+  while (list.next == nullptr) {
+condition.wait(lk);
+  }
+
+  if (list.next == nullptr) {
+while (list.next == nullptr) {
+  condition.wait(lk);
+}
+  }
+  using durtype = std::chrono::duration;
+  durtype dur = std::chrono::duration();
+  if (list.next == nullptr) {
+condition.wait_for(lk, dur);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait_for' should be placed inside a while statement or used with a condition parameter [bugprone-spuriously-wake-up-functions]
+  }
+  if (list.next == nullptr) {
+condition.wait_for(lk, dur, [] { return 1; });
+  }
+  auto now = std::chrono::system_clock::now();
+  if (list.next == nullptr) {
+condition.wait_until(lk, now);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait_until' should be placed inside a while statement or used with a condition parameter [bugprone-spuriously-wake-up-functions]
+  }
+  if (list.next == nullptr) {
+condition.wait_until(lk, now, [] { return 1; });
+  }
+}
+
+typedef struct mtx_t {
+} mtx_t;
+typedef struct cnd_t {
+} cnd_t;
+struct timespec {};
+
+int cnd_wait(cnd_t *cond, mtx_t *mutex){};
+int cnd_timedwait(cnd_t *cond, mtx_t *mutex,
+  const struct timespec *time_point){};
+
+struct Node1 list_c;
+static mtx_t lock;
+static cnd_t condition_c;
+struct timespec ts;
+
+void consume_list_element(void) {
+
+  if (list_c.next == NULL) {
+if (0 != cnd_wait(_c, )) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 'cnd_wait' should be placed inside a while statement [bugprone-spuriously-wake-up-functions]
+}
+  }
+  while (list_c.next == NULL) {
+if (0 != cnd_wait(_c, )) {
+}
+  }
+  if (list_c.next == NULL) {
+cnd_wait(_c, );
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'cnd_wait' should be placed inside a while statement [bugprone-spuriously-wake-up-functions]
+  }
+  while (list.next == NULL) {
+cnd_wait(_c, );
+  }
+  if (list_c.next == NULL) {
+if (0 != cnd_timedwait(_c, , )) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 'cnd_timedwait' should be placed inside a while statement [bugprone-spuriously-wake-up-functions]
+}
+  }
+  while (list_c.next == NULL) {
+if (0 != cnd_timedwait(_c, , )) {
+}
+  }
+  if (list_c.next == NULL) {
+cnd_timedwait(_c, , );
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'cnd_timedwait' should be placed inside a while statement [bugprone-spuriously-wake-up-functions]
+  }
+  while (list.next == NULL) {
+cnd_timedwait(_c, , );
+  }
+}
Index: clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/ratio.h

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/bugprone-spuriously-wake-up-functions.rst:4
+bugprone-spuriously-wake-up-functions
+=
+

Please make length same as in title.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876



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

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Kocsis Ábel via Phabricator via cfe-commits]

abelkocsis updated this revision to Diff 235180.
abelkocsis added a comment.

Checker is moved to bugprone section, tests added. C checker is improved, and 
documentations are synchronised.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/SpuriouslyWakeUpFunctionsCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/bugprone-spuriously-wake-up-functions.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con36-c.rst
  clang-tools-extra/docs/clang-tidy/checks/cert-con54-cpp.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/chrono.h
  
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/condition_variable.h
  
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/cstdint.h
  
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/mutex.h
  
clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/ratio.h
  clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp

Index: clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/bugprone-spuriously-wake-up-functions.cpp
@@ -0,0 +1,79 @@
+// RUN: %check_clang_tidy %s bugprone-spuriously-wake-up-functions %t -- -- -I %S/Inputs/bugprone-spuriously-wake-up-functions/
+#include "condition_variable.h"
+#define NULL 0
+
+struct Node1 {
+  void *Node1;
+  struct Node1 *next;
+};
+
+static Node1 list;
+static std::mutex m;
+static std::condition_variable condition;
+
+void consume_list_element(std::condition_variable ) {
+  std::unique_lock lk(m);
+
+  if (list.next == nullptr) {
+condition.wait(lk);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait' should be placed inside a while statement or used with a condition parameter [bugprone-spuriously-wake-up-functions]
+  }
+
+  while (list.next == nullptr) {
+condition.wait(lk);
+  }
+
+  if (list.next == nullptr) {
+while (list.next == nullptr) {
+  condition.wait(lk);
+}
+  }
+  using durtype = std::chrono::duration;
+  durtype dur = std::chrono::duration();
+  if (list.next == nullptr) {
+condition.wait_for(lk, dur);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait_for' should be placed inside a while statement or used with a condition parameter [bugprone-spuriously-wake-up-functions]
+  }
+  if (list.next == nullptr) {
+condition.wait_for(lk, dur, [] { return 1; });
+  }
+  auto now = std::chrono::system_clock::now();
+  if (list.next == nullptr) {
+condition.wait_until(lk, now);
+// CHECK-MESSAGES: :[[@LINE-1]]:15: warning: 'wait_until' should be placed inside a while statement or used with a condition parameter [bugprone-spuriously-wake-up-functions]
+  }
+  if (list.next == nullptr) {
+condition.wait_until(lk, now, [] { return 1; });
+  }
+}
+
+typedef struct mtx_t {
+} mtx_t;
+typedef struct cnd_t {
+} cnd_t;
+
+int cnd_wait(cnd_t *cond, mtx_t *mutex){};
+
+struct Node1 list_c;
+static mtx_t lock;
+static cnd_t condition_c;
+
+void consume_list_element(void) {
+
+  if (list_c.next == NULL) {
+if (0 != cnd_wait(_c, )) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: 'cnd_wait' should be placed inside a while statement [bugprone-spuriously-wake-up-functions]
+}
+  }
+  while (list_c.next == NULL) {
+if (0 != cnd_wait(_c, )) {
+}
+  }
+  if (list_c.next == NULL) {
+cnd_wait(_c, );
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: 'cnd_wait' should be placed inside a while statement [bugprone-spuriously-wake-up-functions]
+  }
+  while (list.next == NULL) {
+cnd_wait(_c, );
+  }
+}
Index: clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/ratio.h
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/Inputs/bugprone-spuriously-wake-up-functions/ratio.h
@@ -0,0 +1,28 @@
+#include "cstdint.h"
+
+namespace std {
+
+template 
+class ratio {
+public:
+  static constexpr intmax_t num = 0;
+  static constexpr intmax_t den = 0;
+  typedef ratio type;
+};
+
+template 
+struct ratio_equal;
+template 
+struct ratio_not_equal;
+template 
+struct ratio_less;
+template 
+struct ratio_less_equal;
+template 
+struct ratio_greater;
+template 
+struct ratio_greater_equal;
+
+typedef ratio<1, 1000> milli;
+
+} // namespace std
\ No newline at end of file
Index: 

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp:45-46
 "misc-redundant-expression");
+CheckFactories.registerCheck(
+"misc-spuriously-wake-up-functions");
 CheckFactories.registerCheck("misc-static-assert");

If we want to expose this check outside of the CERT module, I think it should 
go into `bugprone` rather than `misc`.



Comment at: 
clang-tools-extra/clang-tidy/misc/SpuriouslyWakeUpFunctionsCheck.cpp:62
+auto hasWaitDescendantC =
+hasDescendant(callExpr(callee(functionDecl(allOf(hasName("cnd_wait"),
+ 
parameterCountIs(2)

What about `cnd_timedwait`?



Comment at: 
clang-tools-extra/clang-tidy/misc/SpuriouslyWakeUpFunctionsCheck.h:19
+/// Finds ``cnd_wait`` or `wait` function calls in an ``IfStmt`` and tries to
+/// replace it with ``WhileStm``.
+///

WhileStm -> WhileStmt



Comment at: 
clang-tools-extra/test/clang-tidy/misc-spuriously-wake-up-functions.cpp:1
+// RUN: %check_clang_tidy %s misc-spuriously-wake-up-functions %t -- -- -I 
%S/../../../libcxx/include/
+

Eugene.Zelenko wrote:
> What will happen if libcxx is not part build/source tree?
Also, there are no tests for the C functionality yet.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876



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

[PATCH] D70876: [clang-tidy] Add spuriously-wake-up-functions check

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

I don't think that //misc// module should be used when checks belong to //cert//




Comment at: 
clang-tools-extra/clang-tidy/misc/SpuriouslyWakeUpFunctionsCheck.cpp:77
+const MatchFinder::MatchResult ) {
+
+  const auto *MatchedWait = Result.Nodes.getNodeAs("wait");

Unnecessary empty line.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:97
 
+- New alias :doc:`cert-con36-c
+  ` to

Please move into aliases section (in alphabetical order). Same below.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:192
 
+- New :doc:`misc-spuriously-wake-up-functions
+  ` check.

Please use alphabetical order.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:195
+
+  Finds ``cnd_wait`` or ``wait`` function calls when the function is not
+  invoked from a loop that checks whether a condition predicate holds or the

Please synchronize with first statement in documentation.



Comment at: 
clang-tools-extra/test/clang-tidy/misc-spuriously-wake-up-functions.cpp:1
+// RUN: %check_clang_tidy %s misc-spuriously-wake-up-functions %t -- -- -I 
%S/../../../libcxx/include/
+

What will happen if libcxx is not part build/source tree?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D70876



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