WebRTC Code Reviews
Help | Chromium Project | Sign in
(25)

Issue 38889004: Base RWLockWrapper on rtc::SharedExclusiveLock. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 8 months ago by pbos
Modified:
4 years, 8 months ago
Reviewers:
tommi
CC:
webrtc-reviews_webrtc.org, tterriberry, ajm, mflodman, henrika_webrtc
Base URL:
http://webrtc.googlecode.com/svn/trunk
Project:
webrtc
Visibility:
Public.

Description

Base RWLockWrapper on rtc::SharedExclusiveLock. Also moves rtc::Event and rtc::SharedExclusiveLock to rtc_base_approved. R=tommi@webrtc.org BUG= Committed: https://code.google.com/p/webrtc/source/detail?r=8260

Patch Set 1 #

Total comments: 3

Patch Set 2 : build.gn #

Patch Set 3 : replace includes, removed some comments, overrides #

Patch Set 4 : build.gn #

Patch Set 5 : windows check #

Patch Set 6 : stupid debugging #

Patch Set 7 : reversing override/annotations #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+35 lines, -400 lines) Patch
M webrtc/base/BUILD.gn View 1 4 chunks +4 lines, -4 lines 0 comments Download
M webrtc/base/base.gyp View 4 chunks +4 lines, -4 lines 0 comments Download
M webrtc/base/event.h View 1 2 1 chunk +0 lines, -1 line 0 comments Download
M webrtc/base/event.cc View 1 2 3 4 3 chunks +5 lines, -3 lines 0 comments Download
M webrtc/system_wrappers/BUILD.gn View 1 chunk +0 lines, -6 lines 0 comments Download
M webrtc/system_wrappers/interface/rw_lock_wrapper.h View 1 2 2 chunks +0 lines, -6 lines 0 comments Download
M webrtc/system_wrappers/source/rw_lock.cc View 1 2 3 4 5 6 1 chunk +22 lines, -18 lines 3 comments Download
D webrtc/system_wrappers/source/rw_lock_generic.h View 1 chunk +0 lines, -46 lines 0 comments Download
D webrtc/system_wrappers/source/rw_lock_generic.cc View 1 chunk +0 lines, -77 lines 0 comments Download
D webrtc/system_wrappers/source/rw_lock_posix.h View 1 chunk +0 lines, -41 lines 0 comments Download
D webrtc/system_wrappers/source/rw_lock_posix.cc View 1 chunk +0 lines, -51 lines 0 comments Download
D webrtc/system_wrappers/source/rw_lock_win.h View 1 chunk +0 lines, -40 lines 0 comments Download
D webrtc/system_wrappers/source/rw_lock_win.cc View 1 chunk +0 lines, -97 lines 0 comments Download
M webrtc/system_wrappers/system_wrappers.gyp View 1 chunk +0 lines, -6 lines 0 comments Download
Project "webrtc" does not have a commit queue.

Messages

Total messages: 13 (0 generated)
pbos
PTAL, I had to use bypass-hooks due to: ** Presubmit ERRORS ** Referencing source files ...
4 years, 8 months ago (2015-02-04 16:45:32 UTC) #1
tommi
lots of deleted code :) https://review.webrtc.org/38889004/diff/1/webrtc/system_wrappers/source/rw_lock.cc File webrtc/system_wrappers/source/rw_lock.cc (right): https://review.webrtc.org/38889004/diff/1/webrtc/system_wrappers/source/rw_lock.cc#newcode20 webrtc/system_wrappers/source/rw_lock.cc:20: virtual void AcquireLockExclusive() EXCLUSIVE_LOCK_FUNCTION() ...
4 years, 8 months ago (2015-02-04 17:48:22 UTC) #2
pbos
build.gn
4 years, 8 months ago (2015-02-05 08:50:25 UTC) #3
pbos
https://review.webrtc.org/38889004/diff/1/webrtc/system_wrappers/source/rw_lock.cc File webrtc/system_wrappers/source/rw_lock.cc (right): https://review.webrtc.org/38889004/diff/1/webrtc/system_wrappers/source/rw_lock.cc#newcode20 webrtc/system_wrappers/source/rw_lock.cc:20: virtual void AcquireLockExclusive() EXCLUSIVE_LOCK_FUNCTION() OVERRIDE { On 2015/02/04 17:48:22, ...
4 years, 8 months ago (2015-02-05 11:13:24 UTC) #4
pbos
build.gn
4 years, 8 months ago (2015-02-05 11:13:33 UTC) #5
pbos
build.gn
4 years, 8 months ago (2015-02-05 13:32:35 UTC) #6
pbos
windows check
4 years, 8 months ago (2015-02-05 14:01:51 UTC) #7
pbos
OK, I think this should compile fine now, finally. PTAL again. :)
4 years, 8 months ago (2015-02-05 14:42:03 UTC) #8
tommi
lgtm https://review.webrtc.org/38889004/diff/120001/webrtc/system_wrappers/source/rw_lock.cc File webrtc/system_wrappers/source/rw_lock.cc (right): https://review.webrtc.org/38889004/diff/120001/webrtc/system_wrappers/source/rw_lock.cc#newcode38 webrtc/system_wrappers/source/rw_lock.cc:38: return new RwLock(); how much trouble would it ...
4 years, 8 months ago (2015-02-05 15:57:38 UTC) #9
pbos
https://review.webrtc.org/38889004/diff/120001/webrtc/system_wrappers/source/rw_lock.cc File webrtc/system_wrappers/source/rw_lock.cc (right): https://review.webrtc.org/38889004/diff/120001/webrtc/system_wrappers/source/rw_lock.cc#newcode38 webrtc/system_wrappers/source/rw_lock.cc:38: return new RwLock(); On 2015/02/05 15:57:38, tommi wrote: > ...
4 years, 8 months ago (2015-02-06 08:31:55 UTC) #10
pbos
Committed patchset #7 (id:120001) manually as 8260.
4 years, 8 months ago (2015-02-06 08:32:46 UTC) #11
tommi
https://review.webrtc.org/38889004/diff/120001/webrtc/system_wrappers/source/rw_lock.cc File webrtc/system_wrappers/source/rw_lock.cc (right): https://review.webrtc.org/38889004/diff/120001/webrtc/system_wrappers/source/rw_lock.cc#newcode38 webrtc/system_wrappers/source/rw_lock.cc:38: return new RwLock(); On 2015/02/06 08:31:55, pbos wrote: > ...
4 years, 8 months ago (2015-02-06 09:18:55 UTC) #12
pbos
4 years, 8 months ago (2015-02-06 09:20:25 UTC) #13
Message was sent while issue was closed.
On 2015/02/06 09:18:55, tommi wrote:
>
https://review.webrtc.org/38889004/diff/120001/webrtc/system_wrappers/source/...
> File webrtc/system_wrappers/source/rw_lock.cc (right):
> 
>
https://review.webrtc.org/38889004/diff/120001/webrtc/system_wrappers/source/...
> webrtc/system_wrappers/source/rw_lock.cc:38: return new RwLock();
> On 2015/02/06 08:31:55, pbos wrote:
> > On 2015/02/05 15:57:38, tommi wrote:
> > > how much trouble would it be to change this to:
> > > 
> > > scoped_ptr<RWLockWrapper> RWLockWrapper::CreateRWLock() {
> > >   return scoped_ptr<RWLockWrapper>(new RwLock()).Pass();
> > > }
> > 
> > Quite a bit I think, we have a bunch of code using it as non-scoped-ptr so
> we'd
> > have to do a swoop over the codebase.
> 
> OK, in that case and given that this implementation is basically
> SharedExclusiveLock now, we should just switch entirely over to that and get
rid
> of this part.  Side benefits of that cleanup would be fewer allocations,
vtables
> and wrappers :)

Yeah I think that makes sense as well, there's no point having a "wrapper" and a
::Create that takes no parameters.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 245c2c2-tainted