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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits