Page MenuHomePhabricator

[clang-tidy] added cppcoreguidelines-use-raii-locks check
Needs ReviewPublic

Authored by lewmpk on Mar 1 2019, 4:27 AM.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2019, 4:27 AM
lewmpk added a subscriber: Restricted Project.Mar 1 2019, 4:28 AM
MyDeveloperDay added inline comments.
clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
45

Nit: by and large this list looks to be in alphabetical based on the checker name (except the last one), not sure if thats by accident or design

lewmpk updated this revision to Diff 188893.Mar 1 2019, 6:13 AM

fixed ordering of cppcoreguidelines module checks

lewmpk marked an inline comment as done.Mar 1 2019, 6:14 AM

This looks good to me but I would wait for one of @JonasToth or @alexfh to perhaps take a look,

maybe you should add some nested scope example too using the same name, I know the compiler should warn about a shadow variable anyway but

std::mutex m;
m.lock();
{
    std::mutex m;
    m.lock();
    m.unlock();
}
m_unlock();

and what about

std::mutex m1;
std::mutex m2;

m1.lock();
m1.unlock();
m2.lock();
m2.unlock();

and

std::mutex m1;
std::mutex m2;

m1.lock();
m2.lock();
m2.unlock();
m1.unlock();
std::mutex m1;
std::mutex m2;

m1.lock();
m1.unlock();
m2.lock();
m2.unlock();
std::mutex m1;
std::mutex m2;

m1.lock();
m2.lock();
m1.unlock();
m2.unlock();
lewmpk added a comment.Mar 1 2019, 6:24 AM

I'll improve the tests :)

+1 i was just going to comment - this needs much more tests.
Also, it would be really great if you could supply the differential's description.T

clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp
26

This should be a config param

27

Terminology: *this* doesn't match anything.
It's a matcher, yes, but it's just a lambda.
The actual match happens at the end.

clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.h
14–15

Separate with newline

lewmpk updated this revision to Diff 188900.Mar 1 2019, 6:39 AM

fixed nested use case

lewmpk updated this revision to Diff 188901.Mar 1 2019, 6:41 AM

removed unneccesary include

Harbormaster completed remote builds in B28671: Diff 188901.
lewmpk updated this revision to Diff 188908.Mar 1 2019, 7:02 AM

handle other basiclockable types

lewmpk updated this revision to Diff 188910.Mar 1 2019, 7:08 AM

renamed option for cppcoreguidelines-use-raii-locks

lewmpk updated this revision to Diff 188911.Mar 1 2019, 7:11 AM

fixed documentation

lewmpk updated this revision to Diff 188912.Mar 1 2019, 7:13 AM

fixed documentation again

lewmpk updated this revision to Diff 188913.Mar 1 2019, 7:15 AM

fixed documentation formatting

lewmpk marked 3 inline comments as done.Mar 1 2019, 7:17 AM
lewmpk added inline comments.
clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp
27

I was trying to describe its intent, not its action. Does anyone have any suggestions for a clearer comment?

clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.h
14–15

it seems that most checks are this way. it was autogenerated by the `add_new_check.py` script.

lewmpk updated this revision to Diff 188915.Mar 1 2019, 7:21 AM

fixed documentation formatting

lewmpk updated this revision to Diff 188918.Mar 1 2019, 7:29 AM

improved tests

lewmpk updated this revision to Diff 188919.Mar 1 2019, 7:39 AM

support try_lock_for and try_lock_until

lewmpk updated this revision to Diff 188920.Mar 1 2019, 7:42 AM

remove support for try_lock_for and try_lock_until

Ugh, could you please avoid doing lots a tiny changes every 5 minutes ? This causes spam on cfe-commits /:

JonasToth added inline comments.Mar 1 2019, 10:19 AM
clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp
35

If we allow boost, pre c++11 is ok as well.
In general, plz use proper grammar, punctuation and full sentences in comments.

41

please add proper punctutation in comments. we aim for correct text you can read and understand, with proper spelling and grammar.

65

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.

69

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.

clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.h
28

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.

test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp
4

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?
lewmpk updated this revision to Diff 189054.Mar 2 2019, 11:20 AM

refactor and improved tests

lewmpk updated this revision to Diff 189055.Mar 2 2019, 11:57 AM

added support for boost lockable types

lewmpk marked 7 inline comments as done.Mar 2 2019, 12:06 PM
lewmpk added inline comments.
test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp
4

I've added a test case for your example, templates, macros and loops.
I can't catch the case std::mutex m1; std::mutex &m2 = m1; // usage, but i can catch trivial cases.

JonasToth added inline comments.Mar 2 2019, 1:12 PM
test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp
4

Yes, your not supposed to catch those. But i feel things like this should be documented. In theory catching this particular case is possible (we do similar analysis for const.
But it is totally acceptable to leave as is!

JonasToth added inline comments.Mar 2 2019, 1:15 PM
test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp
91

I got another one that I think defeats the analysis:

while (true)
{

my_label:
  m.lock();

  if (some_condition)
    goto my_label;
  
  m.unlock();
}

Usage of goto can interfer and make the situation more complicated. I am not asking you to implement that correctly (not sure if even possible with pure clang-tidy) but I think a pathological example should be documented for the user.

Eugene.Zelenko added inline comments.
clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp
30

Please use static for functions instead of anonymous namespaces. See LLVM coding guidelines.

31

Please don't use auto where return type is not obvious.

45

Unnecessary empty line.

73

Unnecessary empty line.

78

Please don't use auto where return type is not obvious.

79

Please don't use auto where return type is not obvious.

83

Please don't use auto where return type is not obvious.

84

Please don't use auto where return type is not obvious.

90

Please don't use auto where return type is not obvious.

92

Please don't use auto where return type is not obvious.

docs/ReleaseNotes.rst
97

Missing space before ``std::mutex

docs/clang-tidy/checks/cppcoreguidelines-use-raii-locks.rst
7

Please synchronize first statement with Release Notes.

19

Please use single ` for options values.

lewmpk updated this revision to Diff 189078.Mar 3 2019, 3:04 AM
lewmpk marked an inline comment as done.

added example in docs and explicitly specified types for some variables

lewmpk marked an inline comment as done.Mar 3 2019, 4:29 AM
lewmpk added inline comments.
test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp
91

why would this defeat the analysis?

JonasToth added inline comments.Mar 3 2019, 11:18 AM
test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp
91

Because goto allows you to reorder you code-locations, so the ordering of what comes before what is not so ez.

lewmpk updated this revision to Diff 189157.Mar 4 2019, 9:16 AM

match lock() and unlock() calls by decl

JonasToth added inline comments.Mar 7 2019, 4:10 AM
test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp
91

let me restate: You are comparing the occurences of lock and unlock by linenumber, so physical appearance in the source code. goto allows you to jump wildly through your code, so that physical appearance does not match actual control flow.
I am not saying that you need to resolve that (not easily done within clang-tidy), but documenting it is worth it.
And if you mix goto and locking mechanisms, you get what you deserve, which is no help from tooling ;)

IMHO the check is close to being finished. please address the notes and mark them as done if finished with it. So its clear to see whats outstanding.
In my opinion the user-facing documentation misses a "Limitations" sections that shows the artificial goto example, that would show that the used mechanism doesn't handle it.