I'm generally of the opinion that if something has a scope block and a comment, that's a strong indication that it should be a function :)

Usually. But for locks and scoped traces, I prefer this. If this routine gets larger or needs to be be called from multiple paths, I'll move it.

The function is doing three distinct things, and is 50+ lines long, so by common evaluations of whether a function is small, there's a pretty good argument that this is already "larger".

I'm not clear what advantage there is in not pulling out at least the two non-trivial, self-contained blocks into helpers. I'm not going to hold up the PR on this, but I'd like to understand the argument for not breaking this up.

My reasons:

• The entire routine fits on one screen (I suppose this depends on font size and such, but I am willing to bet my setup is not particularly esoteric) and control flow follows the eye from top to bottom. That one routine that fits on one screen has 17 lines of comments and 19 lines of code. For a subjective argument about the size of the routine, I am confident in my argument about as much as you are in yours.
• There are very specific locking requirements around the tasks queue mutex. Say I were to split this into three routines. Looking at just the second routine in isolation, I wouldn't know if the caller is supposed to have already acquired the mutex (in which case the routine would have to manually unlock the same because it is a deadlock opportunity to flush tasks with the mutex acquired). This can be addressed by thread safety annotations which I could not use because of the undecorated std::condition_variable, or, naming the routine FlushTaskNoLock (which I always found confusing because does NoLock mean "don't lock" or "its not locked"?). Each has the described issues.
• I question the assertion that the two (1st and 3rd) are non-trivial and self contained. The triviality is subjective I suppose, but I still need to use the same timepoint in both blocks. Splitting into its own routines would mean extra arguments to that routine for reasons not immediately clear (because making two syscalls would merely be a pessimization).

For these reasons, I assert that splitting this already trivial routine further for readability would be counterproductive. Again, all of this is subjective and if any of these blocks had become non-trivial (using subjective evaluations again), I am sure I would have split the same. I believe adding scopes for RAII or just to make sure variables don't escape the same do no automatically mean a that block must be helper.

I've never seen 'g' used as a prefix for a local static; I would expect 's'.

g comes from Hungarian notation and means "global". This doesn't have global scope, and I have a strong opinion that annotating something that's not a global as if it were is wrong :) Please use s, as I would find g actively confusing.

Fair point. Updated. Will patch the other uses in the engine a separate patch.

This seems unrelated to the PR?

This is necessary after --build-glfw-shell on Mac.

The function is doing three distinct things, and is 50+ lines long, so by common evaluations of whether a function is small, there's a pretty good argument that this is already "larger".

I'm not clear what advantage there is in not pulling out at least the two non-trivial, self-contained blocks into helpers. I'm not going to hold up the PR on this, but I'd like to understand the argument for not breaking this up.

There's no scoped lock; why is there a scope block? (I would argue that the fact that you did this despite in not being any mechanical reason I can see is an indication that it should be broken into helpers.)

Yeah, you are right. I wrote the comments first as pseudocode then filled it out with real code. As noted earlier, I don't think adding scopes means that the block should necessarily be extracted into its own helper routine.

s/we posted/PostTask posts/

(go/avoidwe)

Done.

g comes from Hungarian notation and means "global". This doesn't have global scope, and I have a strong opinion that annotating something that's not a global as if it were is wrong :) Please use s, as I would find g actively confusing.

This is necessary after --build-glfw-shell on Mac.

My reasons:

• The entire routine fits on one screen (I suppose this depends on font size and such, but I am willing to bet my setup is not particularly esoteric) and control flow follows the eye from top to bottom. That one routine that fits on one screen has 17 lines of comments and 19 lines of code. For a subjective argument about the size of the routine, I am confident in my argument about as much as you are in yours.
• There are very specific locking requirements around the tasks queue mutex. Say I were to split this into three routines. Looking at just the second routine in isolation, I wouldn't know if the caller is supposed to have already acquired the mutex (in which case the routine would have to manually unlock the same because it is a deadlock opportunity to flush tasks with the mutex acquired). This can be addressed by thread safety annotations which I could not use because of the undecorated std::condition_variable, or, naming the routine FlushTaskNoLock (which I always found confusing because does NoLock mean "don't lock" or "its not locked"?). Each has the described issues.
• I question the assertion that the two (1st and 3rd) are non-trivial and self contained. The triviality is subjective I suppose, but I still need to use the same timepoint in both blocks. Splitting into its own routines would mean extra arguments to that routine for reasons not immediately clear (because making two syscalls would merely be a pessimization).

For these reasons, I assert that splitting this already trivial routine further for readability would be counterproductive. Again, all of this is subjective and if any of these blocks had become non-trivial (using subjective evaluations again), I am sure I would have split the same. I believe adding scopes for RAII or just to make sure variables don't escape the same do no automatically mean a that block must be helper.

Yeah, you are right. I wrote the comments first as pseudocode then filled it out with real code. As noted earlier, I don't think adding scopes means that the block should necessarily be extracted into its own helper routine.

Done.

Fair point. Updated. Will patch the other uses in the engine a separate patch.