This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Implement cppcoreguidelines CP.52
ClosedPublic

Authored by ccotter on Aug 3 2023, 5:17 PM.

Details

Summary

Flag code that suspends a coroutine while a lock is held.

Diff Detail

Event Timeline

ccotter created this revision.Aug 3 2023, 5:17 PM
Herald added a project: Restricted Project. · View Herald Transcript
ccotter requested review of this revision.Aug 3 2023, 5:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 3 2023, 5:17 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
ccotter updated this revision to Diff 547061.Aug 3 2023, 5:23 PM
  • Specify version
Eugene.Zelenko added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
162

Please keep alphabetical order in new checks list.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/no-suspend-with-lock.rst
7

Please synchronize with statement in Release Notes.

PiotrZSL added inline comments.Aug 4 2023, 1:42 PM
clang-tools-extra/clang-tidy/cppcoreguidelines/NoSuspendWithLockCheck.cpp
25–27

add configuration option for lock types, many big project got own types or wrappers.

clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/no-suspend-with-lock.rst
14

add info to documentation that manual locking/unlocking is not supported.

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/no-suspend-with-lock.cpp
190

add test with lambda, something like:

std::unique_lock<std::mutex> lock(mtx);

auto lambda = [] { co_yeld 0; }

And add test with class defined in function with co_yeld in class, and lock in function.

Consider ignoring template instances, to avoid generating lot of warnings for each instance.

ccotter added inline comments.Aug 5 2023, 1:11 PM
clang-tools-extra/clang-tidy/cppcoreguidelines/NoSuspendWithLockCheck.cpp
25–27

I was just thinking about this case...my code base had its own lock types that I would want to be able to include.

ccotter updated this revision to Diff 547573.Aug 6 2023, 6:21 AM
ccotter marked 5 inline comments as done.
  • Add tests, do not inspect instantiations, add option
PiotrZSL added inline comments.Aug 6 2023, 6:49 AM
clang-tools-extra/clang-tidy/cppcoreguidelines/NoSuspendWithLockCheck.cpp
28

consider using matchers::matchesAnyListedName

ccotter updated this revision to Diff 547585.Aug 6 2023, 7:30 AM
  • Use matchesAnyListedName and fix windows build
PiotrZSL accepted this revision.Aug 6 2023, 1:11 PM

LGTM

This revision is now accepted and ready to land.Aug 6 2023, 1:11 PM
ccotter marked an inline comment as done.Aug 10 2023, 9:30 PM

@PiotrZSL would you mind landing this for me? Thanks

This revision was automatically updated to reflect the committed changes.