[PATCH] D58818: [clang-tidy] added cppcoreguidelines-use-raii-locks check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.


================
Comment at: clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp:35
+void UseRaiiLocksCheck::registerMatchers(MatchFinder *Finder) {
+  // lock_guards require c++11 or later
+  if (!getLangOpts().CPlusPlus11)
----------------
If we allow boost, pre c++11 is ok as well.
In general, plz use proper grammar, punctuation and full sentences in comments.


================
Comment at: clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp:41
+      hasDeclaration(cxxRecordDecl(hasAnyListedName(LockableTypes))))));
+  // Match expressions of type mutex or mutex pointer
+  const auto MutexExpr =
----------------
please add proper punctutation in comments. we aim for correct text you can read and understand, with proper spelling and grammar.


================
Comment at: clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp:65
+
+  const auto LockCallExpr = Result.Nodes.getNodeAs<CXXMemberCallExpr>("lock");
+  const auto UnlockCallExpr =
----------------
please add the `*` for pointers to emphasize the difference between values and pointers.
In general we do not add `const` to values (as i believe is done in later lines), but only for pointers and references.


================
Comment at: clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp:69
+
+  const auto LockObjectName = Lexer::getSourceText(
+      CharSourceRange::getTokenRange(
----------------
Please don't retrieve the name like this. Too error prone and compilatcated.

You can compare use `DeclRefExpr` (https://clang.llvm.org/doxygen/classclang_1_1DeclRefExpr.html) for your `MutexExpr` instead.
>From there you go to the `Decl` and compare on pointer equality.


================
Comment at: clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.h:28
+            "LockableTypes",
+            "::std::mutex;::std::recursive_mutex;::std::timed_mutex;::std::"
+            "recursive_timed_mutex;::std::unique_lock")) {}
----------------
It might be a good idea to add the `boost` types as well? I believe they are interface-compatible, given the std version is derived from them.


================
Comment at: test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp:4
+// Mock implementation of std::mutex
+namespace std {
+struct mutex {
----------------
Please add more tests

What happens for this?
```
void foo() {
  std::mutex m;
  m.lock();
  m.unlock();
  m.lock();
  m.unlock();
  m.try_lock();
  m.lock();
  m.unlock();
}
```

- Please add tests for templates, where the lock-type is a template parameter
- please add tests where the locking happens within macros
- please add tests for usage within loops
- where cases like `std::mutex m1; std::mutex &m2 = m1; // usage`. This should not be diagnosed, right?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D58818