[PATCH] D90747: [clangd] Mark AsyncTaskRunner::runAsync as const

Mon Nov 9 12:53:47 PST 2020

sammccall added a comment.

This isn't a hill I'm going to die on, it doesn't matter a great deal - so if this solves 
But I don't think this is an appropriate use of const - it changes the object in a significant way.

"significant" is vague, but i think "wait()ing on a threadpool will now block" qualifies whereas "destroying (long-lived) ProjectAwareIndex will now block" doesn't.
It's as much about the scope of the object as the nature of the change.

A couple of other examples of "thread pool" classes don't make the "schedule" option const:

- http://threadpool.sourceforge.net/reference/a00024.html
- https://pocoproject.org/docs/Poco.ThreadPool.html

(Boost's threadpool is threadsafe like this one)

In D90747#2383716 <https://reviews.llvm.org/D90747#2383716>, @kadircet wrote:

>> This might be OK or maybe we should put `mutable` at the callsite - can you show the real example?
>
> Oops, sorry forgot to update the stack for this one. The usage is in https://reviews.llvm.org/D90750, line 86 of ProjectAware.cpp specifically.

Yeah, this does seem to me like a place to mark the class as "threadsafe" and the member as `mutable`.

> We definitely can make the AsyncTaskRunner mutable there too, but I would feel bad about mutating a "mutable" member without holding a lock in general, hence this change.

I don't think that's bad - mutating requires access to be synchronized, either externally (holding a lock) or internally (class is threadsafe).
This is why it's safe to mutate (lock) the mutable mutex without holding yet another mutex first: the mutex class is threadsafe.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90747/new/

https://reviews.llvm.org/D90747