[cfe-commits] r164854 - in /cfe/trunk: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td include/clang/Sema/ScopeInfo.h lib/Sema/AnalysisBasedWarnings.cpp lib/Sema/Sema.cpp lib/Sema/SemaDecl.cpp lib/Sema/SemaExpr.

[Jordan Rose]

On Oct 2, 2012, at 14:43 , David Blaikie <dblaikie at gmail.com> wrote:

> On Fri, Sep 28, 2012 at 3:21 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>> Author: jrose
>> Date: Fri Sep 28 17:21:30 2012
>> New Revision: 164854
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=164854&view=rev
>> Log:
>> Add a warning (off by default) for repeated use of the same weak property.
>> 
>> The motivating example:
>> 
>> if (self.weakProp)
>>  use(self.weakProp);
> 
> I wonder if this could be generalized/modified to help for things like
> std::weak_ptr too (obviously weak_ptr would need some kind of
> annotation to specify its test/use operations I suppose).

Interesting, hadn't thought about this. It doesn't seem like it'd be as useful, though...weak_ptrs /always/ have to be locked to be used, and as such there's no obvious way to detect deliberate repeated use vs. the user just mindlessly doing the same thing. Though of course you can just say "ptr.lock()->foo(); ptr.lock()->bar();" in the same way, so maybe there's something there...


>> As with any non-atomic test-then-use, it is possible a weak property to be
>> non-nil at the 'if', but be deallocated by the time it is used. The correct
>> way to write this example is as follows:
>> 
>> id tmp = self.weakProp;
>> if (tmp)
>>  use(tmp);
>> 
>> The warning is controlled by -Warc-repeated-use-of-receiver, and uses the
>> property name and base to determine if the same property on the same object
>> is being accessed multiple times. In cases where the base is more
>> complicated than just a single Decl (e.g. 'foo.bar.weakProp'), it picks a
>> Decl for some degree of uniquing and reports the problem under a subflag,
>> -Warc-maybe-repeated-use-of-receiver. This gives a way to tune the
>> aggressiveness of the warning for a particular project.
>> 
>> The warning is not on by default because it is not flow-sensitive and thus
>> may have a higher-than-acceptable rate of false positives,
> 
> I believe we're generally trying to avoid adding off-by-default
> warnings (Doug seems to feel strongly that if it's off-by-default it's
> simply not good enough). Is there a good reason to make an exception
> here?

Yup, and I agree and am usually on that side of the argument. This warning started as a refinement of another off-by-default warning, though, with a specific request from internal users. It's a reasonable warning when it's working but it does have its false positives, though most of those fall into two major categories: imprecise handling of chained property/variable access (a.foo.bar and b.foo.bar look the same to the warning, though self.foo.bar is different) and lack of flow-sensitivity.

It also adds a minor hit to compile time to build up the weak use tables, even if there are no repeated weak uses at all, and I wasn't as concerned about performance when writing it because I knew it was off-by-default. (It's only active under ARC w/ weak references, though, so it won't slow down non-Objective-C compiles even when it's on.)

But I could be convinced that it should be on-by-default. The cases I saw when running this over a large-ish internal project were often somewhat pedantic but definitely correct, and it's much less noisy than -Wreceiver-is-weak.


>> though it is
>> less noisy than -Wreceiver-is-weak. On the other hand, it will not warn
>> about some cases that may be legitimate issues that -Wreceiver-is-weak
>> will catch, and it does not attempt to reason about methods returning weak
>> values.
>> 
>> Even though this is not a real "analysis-based" check I've put the bug
>> emission code in AnalysisBasedWarnings for two reasons: (1) to run on
>> every kind of code body (function, method, block, or lambda), and (2) to
>> suggest that it may be enhanced by flow-sensitive analysis in the future.
>> 
>> The second (smaller) half of this work is to extend it to weak locals
>> and weak ivars. This should use most of the same infrastructure.
>> 
>> Part of <rdar://problem/12280249>