segoon added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/concurrency/AsyncNoNewThreadsCheck.cpp:22
+ /* C++ std */
+ "::std::async", //
+
----------------
aaron.ballman wrote:
> The trailing comment markers don't really add much.
it's a hack for clang-format, otherwise it contatenates the lines, creating
unmaintainable mess of strings. "One line - one name" is much more suitable.
================
Comment at:
clang-tools-extra/clang-tidy/concurrency/AsyncNoNewThreadsCheck.cpp:33
+
+ /* Linux */
+ "::fork", //
----------------
aaron.ballman wrote:
> If we're going to add these, we should probably also add ones for Win32 and
> Mac OS as well, like `CreateThread`, `CreateRemoteThread`, `_beginthread`,
> `_beginthreadex`, etc.
I don't mind, but I'm not an expert in WinAPI or Windows programming. So, this
part should be by someone with expertise of Windows in separate patches.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/concurrency-async-no-new-threads.rst:8-10
+functions and types. E.g. if the code uses C++ coroutines, it is expected
+that only new coroutines or coroutine-based primitives are created
+instead of heavy system threads.
----------------
aaron.ballman wrote:
> FWIW, this suggests to me that what you really want is a way for APIs to opt
> into this behavior. There's no reason why you wouldn't have a complex system
> that has both threads and coroutines in it, but it does stand to reason that
> you may want to say "this function, and everything called within this
> function, should not create any system threads" in some situations.
>
> The note below helps call out the expectations from the check, but it
> requires the developer to restructure the way they write code pretty
> drastically in order to make the checking behavior more reasonable, which
> does not seem ideal.
I think it is a complex problem, so it should be separated into smaller tasks.
Step one - checks with hardcoded functions/types with user-guided enabling on a
per-file basis. A semi-automated check.
Step two - try to solve other parts of the puzzle. Maybe try to add
[clang:coroutine_safe] tag and teach clang static analyzer to deduce coroutine
safety property and use it for enabling/disabling the cheks. Maybe reuse other
(not yet implemented) heuristics from static analyzer (or other tools) to
identify coroutine functions and check only these functions. I'm not an expert
in static analyzer, so other LLVM developers might find a clever heuristics
when to enable/disable these checks or maybe how to deduce
blacklisted/whitelisted functions/types lists (e.g. for
concurrency-async-{fs,blocking}).
Indeed, the current approach has its own limitations. But it may be a first
step in the right direction.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94622/new/
https://reviews.llvm.org/D94622
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
