[PATCH] D18811: Fix a race condition in support library ThreadPool

[Justin Lebar via llvm-commits]

jlebar added inline comments.

================
Comment at: include/llvm/Support/ThreadPool.h:130
@@ -131,3 +129,3 @@
   /// Signal for the destruction of the pool, asking thread to exit.
-  bool EnableFlag;
+  bool InDestructorFlag;
 #endif
----------------
joker.eph wrote:
> jlebar wrote:
> > joker.eph wrote:
> > > Is this related to the changes here? If not then split it out to a separate NFC patch, reducing the change in this patch to the bug it is actually fixing. Same for renaming `QueueCondition ` to `SubmissionCondition`.
> > This feels pretty unnecessary to me.  We're already changing a bunch of things in this class; who cares if we rename one additional variable in this patch as opposed to in a separate one?  All of these changes are related in that they're attempting to make it less likely that this class will have bugs in the future.
> > 
> > There's a cost to carrying small patches in your queue that are tightly intermingled with a base patch; every time you update the base patch, you have to rebase the dependent patch.
> > 
> > I like small patches too, but there is a line.  :)
> This does not seems unnecessary to me, and this is in line with the request I usually receive on my patches. `git blame` and `git show` are a lot more friendly to analyze why the code is the way this way. As we say `written once, read many`...
> (also minimizing diff helps the review)
> 
> As of the cost you're mentioning, you don't have to carry small patches in your queue as you can commit the NFC/renaming change immediately without going through any review. 
> It seems totally "free" to me so I don't really see why not doing it?
> 
> 
> As we say written once, read many...

Surely that does not apply to blame anywhere close to the degree it applies to code itself.

Since code is read so much more often than blame, we should encourage and welcome cleanups, making them easy for the contributor.  As we do, with post-commit review.

In this case, Jason already is carrying the rename in his tree.  You're asking him to either

a) split it out and commit the rename after he's submitted this patch, in which case he has to carry the changes through review and rebase, as I described, or

b) commit the changes now, before this patch is ready, in which case they may not end up being necessary, depending on the direction this review takes.  (Who knows, maybe we remove this variable.)  Committing unnecessary changes adds noise to the mailing list and wastes everyone's time.

In either case it's not free.


http://reviews.llvm.org/D18811