aaron.ballman added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/concurrency/AsyncNoNewThreadsCheck.cpp:41
+ /* WinAPI */
+ "CreateThread", //
+ "CreateRemoteThread", //
----------------
Missing the `::` in front of all of these identifiers.
================
Comment at:
clang-tools-extra/clang-tidy/concurrency/AsyncNoNewThreadsCheck.cpp:22
+ /* C++ std */
+ "::std::async", //
+
----------------
segoon wrote:
> 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.
I empathize, but clang-format is a moving target that hasn't existed as long as
the rest of the project, so the project has a mismatch of formats. This is why
the usual rule of thumb is: clang-format a full file and accept what it
produces (on new files, like this one) or clang-format your diff (on existing
files). If you don't like what clang-format produces in either situation, we
typically format it by hand and then don't worry about it until the next time
someone touches that code. Of course, I notice that we don't mention
clang-format anywhere in the style guide at all. :-(
I won't insist on a change here, but I also would not be surprised if the
useless comment markers were removed by someone during a "cleanup" either.
================
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.
----------------
segoon wrote:
> 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.
> I admit that other projects may have quite different approach to
> namespaces/modules, though.
That's why I'm hoping you can take some measurements of other projects -- it
wouldn't be good to add a check that's highly specific to one project's coding
style (unless it's a check for a particular style guide that's published
somewhere), and I worry a bit that's what is happening here.
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
