[PATCH] D42174: [clangd] Refactored threading in ClangdServer

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/ClangdServer.h:177
+/// preambles and ASTs) for opened files.
+class Scheduler {
+public:
----------------
ilya-biryukov wrote:
> ilya-biryukov wrote:
> > sammccall wrote:
> > > sammccall wrote:
> > > > This class has important responsibilities beyond threading itself, which "Scheduler" suggests.
> > > > 
> > > > I can't think of a perfectly coherent name, options that seem reasonable:
> > > >  - TUManager - pretty bland/vague, but gets what this class is mostly about
> > > >  - Workshop - kind of a silly metaphor, but unlikely to be confused with something else
> > > >  - Clearinghouse - another silly metaphor, maybe more accurate but more obscure
> > > Worth saying something abouth the threading properties here:
> > > 
> > > - Scheduler is not threadsafe, only the main thread should be providing updates and scheduling tasks.
> > >  - callbacks are run on a large threadpool, and it's appropriate to do slow, blocking work in them
> > Added comments.
> It'd be nice to have some mention of the fact that the class handles threading responsibilities. None of the options seem to capture this.
> I don't have good suggestions either, though.
> 
Rename of the Scheduler seems to be the only thing blocking this patch from landing.
I'm happy to go with either of the suggested alternatives or leave as is, I couldn't come up with anything better.

@sammccall, what option would you prefer?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42174