Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2372513004:
Remove Crit::Scope lock by using atomic bool property (Closed)
|
Created:
4 years, 2 months ago by daniela-webrtc Modified:
4 years, 2 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionRemove Crit::Scope lock by using atomic bool property.
The clang static analyzer seems unable to resolve cpp locks in ObjC code.
As of current time, the clang analyzer has known limitations documented
http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#known-limitations.
From the documentation: "The analysis currently does not do any checking
inside constructors or destructors.
In other words, every constructor and destructor is treated
as if it was annotated with NO_THREAD_SAFETY_ANALYSIS."
This is 'probably' why the analyzer is unable to resolve the lock when
used in ObjC land (the cpp works fine).
The lock can be removed by using atomic property instead.
It's not on performance critical path and we expect updates on just one queue and reads from others. That's why the thread assurance atomic properties bring is enough.
The CL removes rtc_sdk_peerconnection_objc_warnings_config as well as it's no longer needed.
BUG=webrtc:6308
Committed: https://crrev.com/edbae5e0ac5be950a8c4372d420c8229bdc7f161
Cr-Commit-Position: refs/heads/master@{#14450}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Replace nonatomic with atomic property. - It's not on performance critical path and we expect upda… #
Total comments: 3
Patch Set 3 : Re-add instance variable and manually synthesize it. - It's an agreed way of defining properties i… #
Messages
Total messages: 39 (15 generated)
Description was changed from ========== Add a #pragma flag to ignore clang thread-safety warring temporarily. The clang static analyzer seems unable to resolve cpp locks in ObjC code. As of current time, the clang analyzer has known limitations documented http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#known-limitations. From the documentation: "The analysis currently does not do any checking inside constructors or destructors. In other words, every constructor and destructor is treated as if it was annotated with NO_THREAD_SAFETY_ANALYSIS." This is 'probably' why the analyzer is unable to resolve the lock when used in ObjC land (the cpp works fine). Pushing the ignore command on top of the clang diagnostic stack solves this issue. Remove rtc_sdk_peerconnection_objc_warnings_config. BUG=6308 ========== to ========== Add a #pragma flag to ignore clang thread-safety warring temporarily. The clang static analyzer seems unable to resolve cpp locks in ObjC code. As of current time, the clang analyzer has known limitations documented http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#known-limitations. From the documentation: "The analysis currently does not do any checking inside constructors or destructors. In other words, every constructor and destructor is treated as if it was annotated with NO_THREAD_SAFETY_ANALYSIS." This is 'probably' why the analyzer is unable to resolve the lock when used in ObjC land (the cpp works fine). Pushing the ignore command on top of the clang diagnostic stack solves this issue. Remove rtc_sdk_peerconnection_objc_warnings_config. BUG=6308 ==========
denicija@webrtc.org changed reviewers: + kthelgason@webrtc.org, magjed@webrtc.org, tkchin@webrtc.org
On 2016/09/27 08:30:42, denicija-webrtc wrote: 👍 LGTM
Description was changed from ========== Add a #pragma flag to ignore clang thread-safety warring temporarily. The clang static analyzer seems unable to resolve cpp locks in ObjC code. As of current time, the clang analyzer has known limitations documented http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#known-limitations. From the documentation: "The analysis currently does not do any checking inside constructors or destructors. In other words, every constructor and destructor is treated as if it was annotated with NO_THREAD_SAFETY_ANALYSIS." This is 'probably' why the analyzer is unable to resolve the lock when used in ObjC land (the cpp works fine). Pushing the ignore command on top of the clang diagnostic stack solves this issue. Remove rtc_sdk_peerconnection_objc_warnings_config. BUG=6308 ========== to ========== Add a #pragma flag to ignore clang thread-safety warring temporarily. The clang static analyzer seems unable to resolve cpp locks in ObjC code. As of current time, the clang analyzer has known limitations documented http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#known-limitations. From the documentation: "The analysis currently does not do any checking inside constructors or destructors. In other words, every constructor and destructor is treated as if it was annotated with NO_THREAD_SAFETY_ANALYSIS." This is 'probably' why the analyzer is unable to resolve the lock when used in ObjC land (the cpp works fine). Pushing the ignore command on top of the clang diagnostic stack solves this issue. Remove rtc_sdk_peerconnection_objc_warnings_config. BUG=webrtc:6308 ==========
lgtm
LGTM, regardless if you land this or use some ObjC locking instead. nit: Can you update the CL description before you land this? 'warring' is misspelled, and we typically have a newline after the first title line and also a newline before the 'BUG=' field. https://codereview.webrtc.org/2372513004/diff/1/webrtc/sdk/objc/Framework/Cla... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2372513004/diff/1/webrtc/sdk/objc/Framework/Cla... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:224: return _isRunning; Is there any ObjC atomic bool we could use for _isRunning instead of the C++ rtc::CritScope?
On 2016/09/27 14:48:46, magjed_webrtc wrote: > LGTM, regardless if you land this or use some ObjC locking instead. > > nit: Can you update the CL description before you land this? 'warring' is > misspelled, and we typically have a newline after the first title line and also > a newline before the 'BUG=' field. > > https://codereview.webrtc.org/2372513004/diff/1/webrtc/sdk/objc/Framework/Cla... > File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): > > https://codereview.webrtc.org/2372513004/diff/1/webrtc/sdk/objc/Framework/Cla... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:224: return > _isRunning; > Is there any ObjC atomic bool we could use for _isRunning instead of the C++ > rtc::CritScope? Yes, just set the property to @property(atomic) BOOL isRunning; And then synthesize instead of implementing explicitly.
Description was changed from ========== Add a #pragma flag to ignore clang thread-safety warring temporarily. The clang static analyzer seems unable to resolve cpp locks in ObjC code. As of current time, the clang analyzer has known limitations documented http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#known-limitations. From the documentation: "The analysis currently does not do any checking inside constructors or destructors. In other words, every constructor and destructor is treated as if it was annotated with NO_THREAD_SAFETY_ANALYSIS." This is 'probably' why the analyzer is unable to resolve the lock when used in ObjC land (the cpp works fine). Pushing the ignore command on top of the clang diagnostic stack solves this issue. Remove rtc_sdk_peerconnection_objc_warnings_config. BUG=webrtc:6308 ========== to ========== Add a #pragma flag to ignore clang thread-safety warring temporarily. The clang static analyzer seems unable to resolve cpp locks in ObjC code. As of current time, the clang analyzer has known limitations documented http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#known-limitations. From the documentation: "The analysis currently does not do any checking inside constructors or destructors. In other words, every constructor and destructor is treated as if it was annotated with NO_THREAD_SAFETY_ANALYSIS." This is 'probably' why the analyzer is unable to resolve the lock when used in ObjC land (the cpp works fine). Pushing the ignore command on top of the clang diagnostic stack solves this issue. Remove rtc_sdk_peerconnection_objc_warnings_config. BUG=webrtc:6308 ==========
On 2016/09/27 15:25:28, tkchin_webrtc wrote: > On 2016/09/27 14:48:46, magjed_webrtc wrote: > > LGTM, regardless if you land this or use some ObjC locking instead. > > > > nit: Can you update the CL description before you land this? 'warring' is > > misspelled, and we typically have a newline after the first title line and > also > > a newline before the 'BUG=' field. > > > > > https://codereview.webrtc.org/2372513004/diff/1/webrtc/sdk/objc/Framework/Cla... > > File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): > > > > > https://codereview.webrtc.org/2372513004/diff/1/webrtc/sdk/objc/Framework/Cla... > > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:224: return > > _isRunning; > > Is there any ObjC atomic bool we could use for _isRunning instead of the C++ > > rtc::CritScope? > > Yes, just set the property to > @property(atomic) BOOL isRunning; > And then synthesize instead of implementing explicitly. I wouldn't be comfortable to set the property to atomic as I'm not certain what would be the performance hit (atomic properties are considered to be slow, at least in comparison with the nonatomic ones). However even if we do the necessary profiling and it turns out to not be a problem, atomic != thread safe (according to Apple's official documentation). What I was thinking that in future we might want to explore using http://opensource.apple.com//source/xnu/xnu-1456.1.26/libkern/libkern/OSAtomic.h Not sure if that will be what we end up using, but I think it's worth exploring. In any case to me that seems like a separate task. What do you think?
On 2016/09/28 08:54:04, denicija-webrtc wrote: > I wouldn't be comfortable to set the property to atomic as I'm not certain what > would be the performance hit (atomic properties are considered to be slow, at > least > in comparison with the nonatomic ones). > However even if we do the necessary profiling and it turns out to not be a > problem, atomic != thread safe (according to Apple's official documentation). > What I was thinking that in future we might want to explore using > http://opensource.apple.com//source/xnu/xnu-1456.1.26/libkern/libkern/OSAtomic.h > Not sure if that will be what we end up using, but I think it's worth exploring. > In any case to me that seems like a separate task. > What do you think? Interesting, I did not know that atomic properties are not thread-safe! OSAtomic looks very interesting, but it's just as much C++ as rtc::CritScope right?
On 2016/09/28 09:09:39, kthelgason wrote: > On 2016/09/28 08:54:04, denicija-webrtc wrote: > > I wouldn't be comfortable to set the property to atomic as I'm not certain > what > > would be the performance hit (atomic properties are considered to be slow, at > > least > > in comparison with the nonatomic ones). > > However even if we do the necessary profiling and it turns out to not be a > > problem, atomic != thread safe (according to Apple's official documentation). > > What I was thinking that in future we might want to explore using > > > http://opensource.apple.com//source/xnu/xnu-1456.1.26/libkern/libkern/OSAtomic.h > > Not sure if that will be what we end up using, but I think it's worth > exploring. > > In any case to me that seems like a separate task. > > What do you think? > > Interesting, I did not know that atomic properties are not thread-safe! OSAtomic > looks very interesting, but it's just as much C++ as rtc::CritScope right? Performance hit shouldn't matter for isRunning because it's a not a performance critical code path and should be negligible. atomic != threadsafe but in this case it can be acceptable because it is set from only one queue, but read on others. Is there a difference between OSAtomic and using an atomic property?
Description was changed from ========== Add a #pragma flag to ignore clang thread-safety warring temporarily. The clang static analyzer seems unable to resolve cpp locks in ObjC code. As of current time, the clang analyzer has known limitations documented http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#known-limitations. From the documentation: "The analysis currently does not do any checking inside constructors or destructors. In other words, every constructor and destructor is treated as if it was annotated with NO_THREAD_SAFETY_ANALYSIS." This is 'probably' why the analyzer is unable to resolve the lock when used in ObjC land (the cpp works fine). Pushing the ignore command on top of the clang diagnostic stack solves this issue. Remove rtc_sdk_peerconnection_objc_warnings_config. BUG=webrtc:6308 ========== to ========== Add a #pragma flag to ignore clang thread-safety warning temporarily. The clang static analyzer seems unable to resolve cpp locks in ObjC code. As of current time, the clang analyzer has known limitations documented http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#known-limitations. From the documentation: "The analysis currently does not do any checking inside constructors or destructors. In other words, every constructor and destructor is treated as if it was annotated with NO_THREAD_SAFETY_ANALYSIS." This is 'probably' why the analyzer is unable to resolve the lock when used in ObjC land (the cpp works fine). Pushing the ignore command on top of the clang diagnostic stack solves this issue. Remove rtc_sdk_peerconnection_objc_warnings_config. BUG=webrtc:6308 ==========
On 2016/09/28 09:31:20, tkchin_webrtc wrote: > On 2016/09/28 09:09:39, kthelgason wrote: > > On 2016/09/28 08:54:04, denicija-webrtc wrote: > > > I wouldn't be comfortable to set the property to atomic as I'm not certain > > what > > > would be the performance hit (atomic properties are considered to be slow, > at > > > least > > > in comparison with the nonatomic ones). > > > However even if we do the necessary profiling and it turns out to not be a > > > problem, atomic != thread safe (according to Apple's official > documentation). > > > What I was thinking that in future we might want to explore using > > > > > > http://opensource.apple.com//source/xnu/xnu-1456.1.26/libkern/libkern/OSAtomic.h > > > Not sure if that will be what we end up using, but I think it's worth > > exploring. > > > In any case to me that seems like a separate task. > > > What do you think? > > > > Interesting, I did not know that atomic properties are not thread-safe! > OSAtomic > > looks very interesting, but it's just as much C++ as rtc::CritScope right? Correct. Might be easier to wrap in ObjC though. > Performance hit shouldn't matter for isRunning because it's a not a performance > critical code path and should be negligible. atomic != threadsafe but in this > case it can be acceptable because it is set from only one queue, but read on > others. That makes sense in that case. I'll update as proposed. > Is there a difference between OSAtomic and using an atomic property? You are right, the atomic property uses spin locks from the OSAtomic toolbox.
Description was changed from ========== Add a #pragma flag to ignore clang thread-safety warning temporarily. The clang static analyzer seems unable to resolve cpp locks in ObjC code. As of current time, the clang analyzer has known limitations documented http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#known-limitations. From the documentation: "The analysis currently does not do any checking inside constructors or destructors. In other words, every constructor and destructor is treated as if it was annotated with NO_THREAD_SAFETY_ANALYSIS." This is 'probably' why the analyzer is unable to resolve the lock when used in ObjC land (the cpp works fine). Pushing the ignore command on top of the clang diagnostic stack solves this issue. Remove rtc_sdk_peerconnection_objc_warnings_config. BUG=webrtc:6308 ========== to ========== The clang static analyzer seems unable to resolve cpp locks in ObjC code. As of current time, the clang analyzer has known limitations documented http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#known-limitations. From the documentation: "The analysis currently does not do any checking inside constructors or destructors. In other words, every constructor and destructor is treated as if it was annotated with NO_THREAD_SAFETY_ANALYSIS." This is 'probably' why the analyzer is unable to resolve the lock when used in ObjC land (the cpp works fine). The lock can be removed by using atomic property instead. It's not on performance critical path and we expect updates on just one queue and reads from others. That's why the thread assurance atomic properties bring is enough. The CL removes rtc_sdk_peerconnection_objc_warnings_config as well as it's no longer needed. BUG=webrtc:6308 ==========
Mind taking a look at this implementation?
lgtm https://codereview.webrtc.org/2372513004/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2372513004/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:113: @synthesize hasStarted = _hasStarted; So this @synthesize step isn't needed? Maybe remove it for the other variables as well, in this CL or in a separate CL.
https://codereview.webrtc.org/2372513004/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2372513004/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:113: @synthesize hasStarted = _hasStarted; On 2016/09/28 13:56:36, magjed_webrtc wrote: > So this @synthesize step isn't needed? Maybe remove it for the other variables > as well, in this CL or in a separate CL. The approach we've gone in this codebase is to explicitly synthesize everything instead of relying on auto-synthesis. It prevents cases where things are accidentally added but never used.
https://codereview.webrtc.org/2372513004/diff/20001/webrtc/sdk/objc/Framework... File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): https://codereview.webrtc.org/2372513004/diff/20001/webrtc/sdk/objc/Framework... webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:113: @synthesize hasStarted = _hasStarted; On 2016/09/28 14:17:37, tkchin_webrtc wrote: > On 2016/09/28 13:56:36, magjed_webrtc wrote: > > So this @synthesize step isn't needed? Maybe remove it for the other variables > > as well, in this CL or in a separate CL. > > The approach we've gone in this codebase is to explicitly synthesize everything > instead of relying on auto-synthesis. It prevents cases where things are > accidentally added but never used. Can you elaborate a bit on this part? Wouldn't the compiler throw an error for unused properties? Or am I misunderstanding your statement?
On 2016/09/28 14:38:42, denicija-webrtc wrote: > https://codereview.webrtc.org/2372513004/diff/20001/webrtc/sdk/objc/Framework... > File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): > > https://codereview.webrtc.org/2372513004/diff/20001/webrtc/sdk/objc/Framework... > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:113: @synthesize > hasStarted = _hasStarted; > On 2016/09/28 14:17:37, tkchin_webrtc wrote: > > On 2016/09/28 13:56:36, magjed_webrtc wrote: > > > So this @synthesize step isn't needed? Maybe remove it for the other > variables > > > as well, in this CL or in a separate CL. > > > > The approach we've gone in this codebase is to explicitly synthesize > everything > > instead of relying on auto-synthesis. It prevents cases where things are > > accidentally added but never used. > > Can you elaborate a bit on this part? Wouldn't the compiler throw an error for > unused properties? Or am I misunderstanding your statement? I'm not sure how compiler would be able to know in the general case. A client may choose to not use a declared property because it's not relevant to their particular use case, and that property may be auto-synthesized. But that doesn't make it invalid. What I'm referring to is sometimes we mistakenly add properties we didn't intend to be in a final CL review. Explicit synthesis usually helps with that because otherwise compiler will complain that you didn't synthesize it (we leave that warning on).
On 2016/09/28 14:42:55, tkchin_webrtc wrote: > On 2016/09/28 14:38:42, denicija-webrtc wrote: > > > https://codereview.webrtc.org/2372513004/diff/20001/webrtc/sdk/objc/Framework... > > File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): > > > > > https://codereview.webrtc.org/2372513004/diff/20001/webrtc/sdk/objc/Framework... > > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:113: > @synthesize > > hasStarted = _hasStarted; > > On 2016/09/28 14:17:37, tkchin_webrtc wrote: > > > On 2016/09/28 13:56:36, magjed_webrtc wrote: > > > > So this @synthesize step isn't needed? Maybe remove it for the other > > variables > > > > as well, in this CL or in a separate CL. > > > > > > The approach we've gone in this codebase is to explicitly synthesize > > everything > > > instead of relying on auto-synthesis. It prevents cases where things are > > > accidentally added but never used. > > > > Can you elaborate a bit on this part? Wouldn't the compiler throw an error for > > unused properties? Or am I misunderstanding your statement? > > I'm not sure how compiler would be able to know in the general case. A client > may choose to not use a declared property because it's not relevant to their > particular use case, and that property may be auto-synthesized. But that doesn't > make it invalid. > > What I'm referring to is sometimes we mistakenly add properties we didn't intend > to be in a final CL review. Explicit synthesis usually helps with that because > otherwise compiler will complain that you didn't synthesize it (we leave that > warning on). Is the rational that explicit synthesize adds more traction? Sorry just trying to better understand :)
On 2016/09/28 15:10:48, denicija-webrtc wrote: > On 2016/09/28 14:42:55, tkchin_webrtc wrote: > > On 2016/09/28 14:38:42, denicija-webrtc wrote: > > > > > > https://codereview.webrtc.org/2372513004/diff/20001/webrtc/sdk/objc/Framework... > > > File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm (right): > > > > > > > > > https://codereview.webrtc.org/2372513004/diff/20001/webrtc/sdk/objc/Framework... > > > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:113: > > @synthesize > > > hasStarted = _hasStarted; > > > On 2016/09/28 14:17:37, tkchin_webrtc wrote: > > > > On 2016/09/28 13:56:36, magjed_webrtc wrote: > > > > > So this @synthesize step isn't needed? Maybe remove it for the other > > > variables > > > > > as well, in this CL or in a separate CL. > > > > > > > > The approach we've gone in this codebase is to explicitly synthesize > > > everything > > > > instead of relying on auto-synthesis. It prevents cases where things are > > > > accidentally added but never used. > > > > > > Can you elaborate a bit on this part? Wouldn't the compiler throw an error > for > > > unused properties? Or am I misunderstanding your statement? > > > > I'm not sure how compiler would be able to know in the general case. A client > > may choose to not use a declared property because it's not relevant to their > > particular use case, and that property may be auto-synthesized. But that > doesn't > > make it invalid. > > > > What I'm referring to is sometimes we mistakenly add properties we didn't > intend > > to be in a final CL review. Explicit synthesis usually helps with that because > > otherwise compiler will complain that you didn't synthesize it (we leave that > > warning on). > > Is the rational that explicit synthesize adds more traction? Sorry just trying > to better understand :) Yeah, explicit synthesis or explicit method implementation usually means that you meant to add the property you did.
On 2016/09/28 15:12:02, tkchin_webrtc wrote: > On 2016/09/28 15:10:48, denicija-webrtc wrote: > > On 2016/09/28 14:42:55, tkchin_webrtc wrote: > > > On 2016/09/28 14:38:42, denicija-webrtc wrote: > > > > > > > > > > https://codereview.webrtc.org/2372513004/diff/20001/webrtc/sdk/objc/Framework... > > > > File webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm > (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/2372513004/diff/20001/webrtc/sdk/objc/Framework... > > > > webrtc/sdk/objc/Framework/Classes/avfoundationvideocapturer.mm:113: > > > @synthesize > > > > hasStarted = _hasStarted; > > > > On 2016/09/28 14:17:37, tkchin_webrtc wrote: > > > > > On 2016/09/28 13:56:36, magjed_webrtc wrote: > > > > > > So this @synthesize step isn't needed? Maybe remove it for the other > > > > variables > > > > > > as well, in this CL or in a separate CL. > > > > > > > > > > The approach we've gone in this codebase is to explicitly synthesize > > > > everything > > > > > instead of relying on auto-synthesis. It prevents cases where things are > > > > > accidentally added but never used. > > > > > > > > Can you elaborate a bit on this part? Wouldn't the compiler throw an error > > for > > > > unused properties? Or am I misunderstanding your statement? > > > > > > I'm not sure how compiler would be able to know in the general case. A > client > > > may choose to not use a declared property because it's not relevant to their > > > particular use case, and that property may be auto-synthesized. But that > > doesn't > > > make it invalid. > > > > > > What I'm referring to is sometimes we mistakenly add properties we didn't > > intend > > > to be in a final CL review. Explicit synthesis usually helps with that > because > > > otherwise compiler will complain that you didn't synthesize it (we leave > that > > > warning on). > > > > Is the rational that explicit synthesize adds more traction? Sorry just trying > > to better understand :) > > Yeah, explicit synthesis or explicit method implementation usually means that > you meant to add the property you did. In that case I'll return the synthesize part.
Now should be fine. Mind giving one (hopefully) final look?
On 2016/09/29 09:41:40, denicija-webrtc wrote: > Now should be fine. > Mind giving one (hopefully) final look? lgtm
The CQ bit was checked by denicija@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kthelgason@webrtc.org, magjed@webrtc.org Link to the patchset: https://codereview.webrtc.org/2372513004/#ps40001 (title: "Re-add instance variable and manually synthesize it. - It's an agreed way of defining properties i…")
The CQ bit was unchecked by denicija@webrtc.org
The CQ bit was checked by denicija@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_baremetal on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_baremetal/builds/...)
Description was changed from ========== The clang static analyzer seems unable to resolve cpp locks in ObjC code. As of current time, the clang analyzer has known limitations documented http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#known-limitations. From the documentation: "The analysis currently does not do any checking inside constructors or destructors. In other words, every constructor and destructor is treated as if it was annotated with NO_THREAD_SAFETY_ANALYSIS." This is 'probably' why the analyzer is unable to resolve the lock when used in ObjC land (the cpp works fine). The lock can be removed by using atomic property instead. It's not on performance critical path and we expect updates on just one queue and reads from others. That's why the thread assurance atomic properties bring is enough. The CL removes rtc_sdk_peerconnection_objc_warnings_config as well as it's no longer needed. BUG=webrtc:6308 ========== to ========== Remove Crit::Scope lock by using atomic bool property. The clang static analyzer seems unable to resolve cpp locks in ObjC code. As of current time, the clang analyzer has known limitations documented http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#known-limitations. From the documentation: "The analysis currently does not do any checking inside constructors or destructors. In other words, every constructor and destructor is treated as if it was annotated with NO_THREAD_SAFETY_ANALYSIS." This is 'probably' why the analyzer is unable to resolve the lock when used in ObjC land (the cpp works fine). The lock can be removed by using atomic property instead. It's not on performance critical path and we expect updates on just one queue and reads from others. That's why the thread assurance atomic properties bring is enough. The CL removes rtc_sdk_peerconnection_objc_warnings_config as well as it's no longer needed. BUG=webrtc:6308 ==========
The CQ bit was checked by denicija@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Remove Crit::Scope lock by using atomic bool property. The clang static analyzer seems unable to resolve cpp locks in ObjC code. As of current time, the clang analyzer has known limitations documented http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#known-limitations. From the documentation: "The analysis currently does not do any checking inside constructors or destructors. In other words, every constructor and destructor is treated as if it was annotated with NO_THREAD_SAFETY_ANALYSIS." This is 'probably' why the analyzer is unable to resolve the lock when used in ObjC land (the cpp works fine). The lock can be removed by using atomic property instead. It's not on performance critical path and we expect updates on just one queue and reads from others. That's why the thread assurance atomic properties bring is enough. The CL removes rtc_sdk_peerconnection_objc_warnings_config as well as it's no longer needed. BUG=webrtc:6308 ========== to ========== Remove Crit::Scope lock by using atomic bool property. The clang static analyzer seems unable to resolve cpp locks in ObjC code. As of current time, the clang analyzer has known limitations documented http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#known-limitations. From the documentation: "The analysis currently does not do any checking inside constructors or destructors. In other words, every constructor and destructor is treated as if it was annotated with NO_THREAD_SAFETY_ANALYSIS." This is 'probably' why the analyzer is unable to resolve the lock when used in ObjC land (the cpp works fine). The lock can be removed by using atomic property instead. It's not on performance critical path and we expect updates on just one queue and reads from others. That's why the thread assurance atomic properties bring is enough. The CL removes rtc_sdk_peerconnection_objc_warnings_config as well as it's no longer needed. BUG=webrtc:6308 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Remove Crit::Scope lock by using atomic bool property. The clang static analyzer seems unable to resolve cpp locks in ObjC code. As of current time, the clang analyzer has known limitations documented http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#known-limitations. From the documentation: "The analysis currently does not do any checking inside constructors or destructors. In other words, every constructor and destructor is treated as if it was annotated with NO_THREAD_SAFETY_ANALYSIS." This is 'probably' why the analyzer is unable to resolve the lock when used in ObjC land (the cpp works fine). The lock can be removed by using atomic property instead. It's not on performance critical path and we expect updates on just one queue and reads from others. That's why the thread assurance atomic properties bring is enough. The CL removes rtc_sdk_peerconnection_objc_warnings_config as well as it's no longer needed. BUG=webrtc:6308 ========== to ========== Remove Crit::Scope lock by using atomic bool property. The clang static analyzer seems unable to resolve cpp locks in ObjC code. As of current time, the clang analyzer has known limitations documented http://clang.llvm.org/docs/ThreadSafetyAnalysis.html#known-limitations. From the documentation: "The analysis currently does not do any checking inside constructors or destructors. In other words, every constructor and destructor is treated as if it was annotated with NO_THREAD_SAFETY_ANALYSIS." This is 'probably' why the analyzer is unable to resolve the lock when used in ObjC land (the cpp works fine). The lock can be removed by using atomic property instead. It's not on performance critical path and we expect updates on just one queue and reads from others. That's why the thread assurance atomic properties bring is enough. The CL removes rtc_sdk_peerconnection_objc_warnings_config as well as it's no longer needed. BUG=webrtc:6308 Committed: https://crrev.com/edbae5e0ac5be950a8c4372d420c8229bdc7f161 Cr-Commit-Position: refs/heads/master@{#14450} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/edbae5e0ac5be950a8c4372d420c8229bdc7f161 Cr-Commit-Position: refs/heads/master@{#14450} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
