Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(473)

Issue 755042: Fixes leak in ThreadLocal.

Can't Edit
Can't Publish+Mail
Start Review
Created:
15 years, 9 months ago by wan
Modified:
10 years, 10 months ago
Reviewers:
fazekasmiklos
CC:
googletestframework_googlegroups.com
Base URL:
http://googletest.googlecode.com/svn/trunk/
Visibility:
Public.

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+84 lines, -21 lines) Patch
M include/gtest/internal/gtest-port.h View 3 chunks +20 lines, -4 lines 1 comment Download
M test/gtest-port_test.cc View 2 chunks +64 lines, -17 lines 1 comment Download

Messages

Total messages: 2
mfazekas
I think it's a good solution if we don't need a general ThreadLocal implementation. I've ...
15 years, 9 months ago (2010-03-26 07:59:22 UTC) #1
wan
15 years, 9 months ago (2010-03-26 15:47:14 UTC) #2
Thanks for the comments, Miklos.  I'll make the Windows-related
changes when we decide to implement ThreadLocal on Windows, as the
whole design may change at that time, making many things moot.
Thanks,

On Fri, Mar 26, 2010 at 12:59 AM,  <FazekasMiklos@gmail.com> wrote:
> I think it's a good solution if we don't need a general ThreadLocal
> implementation. I've added a note about issues with a windows
> implementation that was brought up by Vlad during the review of the
> original patch.
>
>
> http://codereview.appspot.com/755042/diff/1/3
> File include/gtest/internal/gtest-port.h (right):
>
> http://codereview.appspot.com/755042/diff/1/3#newcode1090
> include/gtest/internal/gtest-port.h:1090: // when the thread exits.  Or,
> if the ThreadLocal instance dies in
> There are challenges with implementing destruction of thread locals on
> thread exit on windows. Maybe we can add a comment here that on other
> implementations destruction of thread locals might happen when the
> ThreadLocal instance is destroyed.
>
> http://codereview.appspot.com/755042/diff/1/2
> File test/gtest-port_test.cc (right):
>
> http://codereview.appspot.com/755042/diff/1/2#newcode990
> test/gtest-port_test.cc:990: // should have been destroyed.
> When we'll have a windows implementation this test should be pthread
> only.
>
> http://codereview.appspot.com/755042/show
>



-- 
Zhanyong
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b