segoon added inline comments.
================ Comment at: clang-tools-extra/clang-tidy/concurrency/AsyncNoNewThreadsCheck.cpp:22 + /* C++ std */ + "::std::async", // + ---------------- aaron.ballman wrote: > segoon wrote: > > 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. > Ah, I didn't know that'd change the behavior of clang-format, that's neat! > > FWIW, we usually don't do a whole lot of markup to avoid clang-format issues > (such as the clang-format: on/off markers). Instead, we usually just ignore > the LINT warnings in code review and check the code in as-is. This helps > reduce clutter in the code base. In this case, the comments aren't adding a > ton of clutter so maybe they're fine. But they definitely look odd as a > reader of the code, which is a bit distracting. IMO having an ability to run clang-format for sources is very handy. It makes me sad if running clang-format against the source forces me to set some markers or reverting a part of clang-format changes in the source parts I have never touched. An ability to simply rerun checks/linters is very powerful. ================ Comment at: clang-tools-extra/clang-tidy/concurrency/AsyncNoNewThreadsCheck.cpp:33 + + /* Linux */ + "::fork", // ---------------- aaron.ballman wrote: > segoon wrote: > > 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. > I don't think it needs to be done in separate patches. I can give you the > full list of things I care about. > ``` > CreateThread > (https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createthread) > CreateRemoteThread > (https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createremotethread) > CreateRemoteThreadEx > (https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createremotethreadex) > _beginthread > _beginthreadex > (https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/beginthread-beginthreadex?view=msvc-160) > ``` Oh, great, thank you! Added. ================ 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: > segoon wrote: > > 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. > My fear with the current approach is that I don't think projects usually > split their code in the way this check requires, and so the check will not be > very useful in practice because it will be too chatty. Have you tried running > this check over some large projects that use coroutines to see what the > diagnostic results look like? I haven't conducted a survey, but I can say for Yandex.Taxi where the patch has originated. The codebase is ~350KLOC. The main parts are: 1) a coroutine framework that uses blocking functions / threads creation /etc. inside - concurrency-async-* disabled 2) coroutine libraries consisting of coroutine-only code - concurrency-async-* enabled 3) services with coroutine-only code - concurrency-async-* enabled All 3 parts are clearly separated from each other from the filesystem prospective. Actually I think there are plenty of other invariants that must be met for coroutine code, so separate coroutine/non-coroutine namespaces (and consequently, filesystem separation) is a must. E.g. there are filesystem::RewriteFileContents() and filesystem::blocking::RewriteFileContents() - trivial to realize whether you may call it or not. I admit that other projects may have quite different approach to namespaces/modules, though. 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
