Re: Change to ThreadBase for review

Eric Sunshine <[email protected]> Wed, 7 Aug 2013 10:43:32 -0400
Newsgroups gmane.comp.graphics.crystalspace.devel
Message-ID <CAPig+cQ=1xFa3C=wawbDDr+jOFNJJC3-_KnzUkfO1H+Rq3X82A@mail.gmail.com>
On Tue, Aug 6, 2013 at 9:04 PM, Ralph Campbell <[email protected]> wrote:
> Thanks for the comments.
> I agree with them and I'm used to doing Linux development via patches so
> the splitting & white space comments make sense.

It's not a requirement of this project, but if it's something you're
used to, I personally would be pleased to see commits made in the
style of the Linux kernel or similar projects: one well-defined,
small, easily digestible topic per commit; first line of commit
message a short summary of change; a well written commit message which
explains the problem and justifies the solution. (At least a couple of
us use git for CS work, so commit messages formatted in the style
suggested by git make for nicer display in the git tool ecosystem.)

> I wasn't expecting speedy responses. I probably would have waited a week
> and then done what you suggest, commit and track comments.

Sounds reasonable. Unfortunately, these days, the project is rather
short of developers, so it can be difficult to get reviews or input.
Developers who are around have indicated (off-list) that they are
comfortable with the quality of your patches, so, if you are confident
about your changes, you should feel free to commit them.

> The run state is only being used internally to track whether Wait()
> and such need to clean up state. Since the clean up needs to happen
> whether or not the thread has exited, the IsRunning() call would
> now just indicate whether it was started or not.
> The actual use is that all the callers call Wait() to be sure
> a thread is finished.

Indeed, and a client's use of IsRunning() for anything other would be
subject to race conditions anyhow -- with or without this patch -- so
the new slightly different semantic wouldn't harm any such client (if
there was one) beyond the harm it's already doing to itself by relying
upon IsRunning().

-- ES

------------------------------------------------------------------------------
Get 100% visibility into Java/.NET code with AppDynamics Lite!
It's a free troubleshooting tool designed for production.
Get down to code-level detail for bottlenecks, with <2% overhead. 
Download for free and get started troubleshooting in minutes. 
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk