mwyman marked 4 inline comments as done.
mwyman added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/objc/NSInvocationArgumentLifetimeCheck.cpp:49
+ QT->getScalarTypeKind() == Type::STK_BlockPointer) &&
+ QT.getQualifiers().getObjCLifetime() > Qualifiers::OCL_ExplicitNone;
+}
----------------
stephanemoore wrote:
> I'm not sure but I guess it's okay to assume that the enumeration ordering
> will remain stable?
I don't know why anyone would change the ordering, but FWIW this is the same
approach used by the ARC migrator code.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/objc-nsinvocation-argument-lifetime.m:64-69
+- (void)processInvocation:(NSInvocation *)Invocation {
+ [Invocation getArgument:&Argument atIndex:2];
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation
'-getArgument:atIndex:' should only pass pointers to objects with ownership
__unsafe_unretained [objc-nsinvocation-argument-lifetime]
+ [Invocation getArgument:&self->Argument atIndex:2];
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation
'-getArgument:atIndex:' should only pass pointers to objects with ownership
__unsafe_unretained [objc-nsinvocation-argument-lifetime]
+}
----------------
stephanemoore wrote:
> stephanemoore wrote:
> > How about a test case where we take the address of something on an ivar?
> >
> > For example, I think the code below should not produce a diagnostic
> > finding, right?
> > ```
> > struct Example {
> > __unsafe_unretained id Field;
> > };
> >
> > @interface TestClass : NSObject {
> > struct Example ExampleIvar;
> > }
> >
> > @end
> >
> > @implementation TestClass
> >
> > - (void)processInvocation:(NSInvocation *)Invocation {
> > [Invocation getArgument:&(self->ExampleIvar.Field) atIndex:2];
> > }
> >
> > @end
> > ```
> Maybe worth adding an ivar case that doesn't produce a diagnostic?
This required checking MemberRefExprs too. Added both a matching and
non-matching case here.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/objc-nsinvocation-argument-lifetime.m:64-69
+- (void)processInvocation:(NSInvocation *)Invocation {
+ [Invocation getArgument:&Argument atIndex:2];
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation
'-getArgument:atIndex:' should only pass pointers to objects with ownership
__unsafe_unretained [objc-nsinvocation-argument-lifetime]
+ [Invocation getArgument:&self->Argument atIndex:2];
+ // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: NSInvocation
'-getArgument:atIndex:' should only pass pointers to objects with ownership
__unsafe_unretained [objc-nsinvocation-argument-lifetime]
+}
----------------
mwyman wrote:
> stephanemoore wrote:
> > stephanemoore wrote:
> > > How about a test case where we take the address of something on an ivar?
> > >
> > > For example, I think the code below should not produce a diagnostic
> > > finding, right?
> > > ```
> > > struct Example {
> > > __unsafe_unretained id Field;
> > > };
> > >
> > > @interface TestClass : NSObject {
> > > struct Example ExampleIvar;
> > > }
> > >
> > > @end
> > >
> > > @implementation TestClass
> > >
> > > - (void)processInvocation:(NSInvocation *)Invocation {
> > > [Invocation getArgument:&(self->ExampleIvar.Field) atIndex:2];
> > > }
> > >
> > > @end
> > > ```
> > Maybe worth adding an ivar case that doesn't produce a diagnostic?
> This required checking MemberRefExprs too. Added both a matching and
> non-matching case here.
Added a non-matching ivar case, and in doing so discovered it was often
actually matching the "self" reference, which is an ImplicitParamDecl, itself a
type of VarDecl, which matched the declRefExpr(), leading to even cases that
shouldn't have produced diagnostics to produce them.
Have fixed this by excluding the self case (by ensuring the declRefExpr's
parent is not an implicitCastExpr, which only appears in the AST that I've seen
around the self—or other dereferenced object—reference).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D77571/new/
https://reviews.llvm.org/D77571
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
