Details
- Reviewers
hokein aaron.ballman alexfh JonasToth
Diff Detail
- Repository
- rCTE Clang Tools Extra
- Build Status
Buildable 28753 Build 28752: arc lint + arc unit
Event Timeline
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 |
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();
+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. | |
clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.h | ||
14–15 | Separate with newline |
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. |
Ugh, could you please avoid doing lots a tiny changes every 5 minutes ? This causes spam on cfe-commits /:
clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.cpp | ||
---|---|---|
36 | If we allow boost, pre c++11 is ok as well. | |
42 | please add proper punctutation in comments. we aim for correct text you can read and understand, with proper spelling and grammar. | |
66 | please add the * for pointers to emphasize the difference between values and pointers. | |
70 | 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. | |
clang-tidy/cppcoreguidelines/UseRaiiLocksCheck.h | ||
29 | 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 | ||
5 | 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(); }
|
test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp | ||
---|---|---|
5 | I've added a test case for your example, templates, macros and loops. |
test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp | ||
---|---|---|
5 | 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. |
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. |
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. |
test/clang-tidy/cppcoreguidelines-use-raii-locks.cpp | ||
---|---|---|
91 | why would this defeat the analysis? |
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. |
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. |
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.
bump - I know this is really old, but @lewmpk do you plan on finishing up the last remaining comments?
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