Cleanup thread_local_storage on Windows

Cleanup thread_local_storage on Windows

Remove statics that are windows only from the
header file, and relocated them the windows only file.

FIx up a few names to make things a bit more readable.

r=rvargas

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=142511

[jar (doing other things), 2012-05-31 18:32:22]

This is a pretty trivial fixup that was sitting in my tree for a month.

I just moved statics members that were Windows specific out of a global class
definition, and made them private in the implementation class (for Windows).

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
File base/threading/thread_local_storage_win.cc (right):

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:37: long g_last_used_tls_key = 0;
Note that this is consistently one-less than the old static |tls_max_|, which
held the first unused key (index).

[rvargas (doing something else), 2012-06-01 18:54:36]

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
File base/threading/thread_local_storage_win.cc (right):

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:14: namespace {
We Should move this namespace out of base::. That would follow the usual
pattern.

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:15: // We use an anonymous namespace
here rather than poluting the global header
Drop this comment :)

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:32: long g_windows_native_tls_key =
TLS_OUT_OF_INDEXES;
nit: this sounds a little redundant to me (the windows_native part), given that
it is on the anon namespace of a win specific file :). But of course, keep it if
you feel strong about it.

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:36: // with an unallocated TLS slot.
Isn't this comment stale? I mean, 0 is not the last one but the next one (and
maybe the name should reflect that), but now the thing about skipping one, and
especially avoiding confusion with an unallocated TLS  is just... confusing.

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:106:
!TlsGetValue(g_windows_native_tls_key))
nit: now we need {}

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:151: void WinThreadExit() {
And this code should move up to the anon namespace (either before check in this
CL or on a follow up CL).

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:240: base::WinThreadExit();
... and this would become just WinThreadExit();

[jar (doing other things), 2012-06-05 17:45:45]

Changes per comments by rvargas.

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
File base/threading/thread_local_storage_win.cc (right):

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:14: namespace {
On 2012/06/01 18:54:36, rvargas wrote:
> We Should move this namespace out of base::. That would follow the usual
> pattern.

Done.

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:15: // We use an anonymous namespace
here rather than poluting the global header
On 2012/06/01 18:54:36, rvargas wrote:
> Drop this comment :)

Done.

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:32: long g_windows_native_tls_key =
TLS_OUT_OF_INDEXES;
On 2012/06/01 18:54:36, rvargas wrote:
> nit: this sounds a little redundant to me (the windows_native part), given
that
> it is on the anon namespace of a win specific file :). But of course, keep it
if
> you feel strong about it.

Done.

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:36: // with an unallocated TLS slot.
The comment is (now) perchance more meaningful than it used to be with the other
varible.  We "skip 0" which is why me "claim it was used" and set
g_last_used_tls_key = 0.

0 is *not* the next one.  The next one to use will be 1.  We will never use 0
(but here we act like we did).

On 2012/06/01 18:54:36, rvargas wrote:
> Isn't this comment stale? I mean, 0 is not the last one but the next one (and
> maybe the name should reflect that), but now the thing about skipping one, and
> especially avoiding confusion with an unallocated TLS  is just... confusing.

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:106:
!TlsGetValue(g_windows_native_tls_key))
Name change collapsed line... so we don't need curlies now ;-).

On 2012/06/01 18:54:36, rvargas wrote:
> nit: now we need {}

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:151: void WinThreadExit() {
On 2012/06/01 18:54:36, rvargas wrote:
> And this code should move up to the anon namespace (either before check in
this
> CL or on a follow up CL).

Done.

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:240: base::WinThreadExit();
On 2012/06/01 18:54:36, rvargas wrote:
> ... and this would become just WinThreadExit();

Done.

[rvargas (doing something else), 2012-06-05 18:25:01]

lgtm

https://chromiumcodereview.appspot.com/8890003/diff/18003/base/threading/thre...
File base/threading/thread_local_storage_win.cc (right):

https://chromiumcodereview.appspot.com/8890003/diff/18003/base/threading/thre...
base/threading/thread_local_storage_win.cc:36: // with an unallocated TLS slot.
Well... the comment is still confusing (and I was confused last time I reviewed
the same change, a while ago).

Even if before it was not totally precise, seeing "the code is skipping 0
because it sets the variable to 1" was in agreement with the comment, and then
the reader just moved on (and faced the funky -1 later on).

Now the comment says "we skip 0", but it's hard to see the code doing that. How
will not skip 0, setting the variable to -1? that would be even more weird. What
does "confused with an unallocated TLS slot" means in this context?

So I think the comment needs to say either more, or less than what is saying now
:)

On 2012/06/05 17:45:45, jar wrote:
> The comment is (now) perchance more meaningful than it used to be with the
other
> varible.  We "skip 0" which is why me "claim it was used" and set
> g_last_used_tls_key = 0.
> 
> 0 is *not* the next one.  The next one to use will be 1.  We will never use 0
> (but here we act like we did).
> 
> On 2012/06/01 18:54:36, rvargas wrote:
> > Isn't this comment stale? I mean, 0 is not the last one but the next one
(and
> > maybe the name should reflect that), but now the thing about skipping one,
and
> > especially avoiding confusion with an unallocated TLS  is just... confusing.
>

[jar (doing other things), 2012-06-07 18:23:45]

Changes made per comment by rvargas.

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
File base/threading/thread_local_storage_win.cc (right):

http://codereview.chromium.org/8890003/diff/18003/base/threading/thread_local...
base/threading/thread_local_storage_win.cc:36: // with an unallocated TLS slot.
On 2012/06/05 18:25:01, rvargas wrote:
> Well... the comment is still confusing (and I was confused last time I
reviewed
> the same change, a while ago).
> 
> Even if before it was not totally precise, seeing "the code is skipping 0
> because it sets the variable to 1" was in agreement with the comment, and then
> the reader just moved on (and faced the funky -1 later on).
> 
> Now the comment says "we skip 0", but it's hard to see the code doing that.
How
> will not skip 0, setting the variable to -1? that would be even more weird.
What
> does "confused with an unallocated TLS slot" means in this context?
> 
> So I think the comment needs to say either more, or less than what is saying
now
> :)
> 
> On 2012/06/05 17:45:45, jar wrote:
> > The comment is (now) perchance more meaningful than it used to be with the
> other
> > varible.  We "skip 0" which is why me "claim it was used" and set
> > g_last_used_tls_key = 0.
> > 
> > 0 is *not* the next one.  The next one to use will be 1.  We will never use
0
> > (but here we act like we did).
> > 
> > On 2012/06/01 18:54:36, rvargas wrote:
> > > Isn't this comment stale? I mean, 0 is not the last one but the next one
> (and
> > > maybe the name should reflect that), but now the thing about skipping one,
> and
> > > especially avoiding confusion with an unallocated TLS  is just...
confusing.
> > 
> 

Done.

[commit-bot: I haz the power, 2012-06-07 18:24:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jar@chromium.org/8890003/36003

[commit-bot: I haz the power, 2012-06-07 20:07:54]

Try job failure for 8890003-36003 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-06-15 20:49:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jar@chromium.org/8890003/36003

[commit-bot: I haz the power, 2012-06-15 22:44:27]

Change committed as 142511