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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
