[cfe-commits] r78029 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/Sema.h lib/Sema/SemaDeclObjC.cpp test/SemaObjC/warn-superclass-method-mismatch.m

[Fariborz Jahanian]

On Aug 3, 2009, at 9:57 PM, Chris Lattner wrote:

>
> On Aug 3, 2009, at 6:07 PM, Fariborz Jahanian wrote:
>
>> Author: fjahanian
>> Date: Mon Aug  3 20:07:16 2009
>> New Revision: 78029
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=78029&view=rev
>> Log:
>> Compare matching selectors in current and
>> super class(s) and warn on any parameter
>> type mismatch if potentially unsafe.
>
> Nice,
>
>>
>> +void Sema::CompareMethodParamsInBaseAndSuper(Decl *ClassDecl,
>
> Please add a comment explaining what this does.
>
>> +                                             ObjCMethodDecl *Method,
>> +                                             bool IsInstance)  {
>> +  if (ObjCInterfaceDecl *ID =  
>> dyn_cast<ObjCInterfaceDecl>(ClassDecl))
>> +    while (ObjCInterfaceDecl *SD = ID->getSuperClass()) {
>> +      if (ObjCMethodDecl *SuperMethodDecl =
>> +          SD->lookupMethod(Method->getSelector(), IsInstance)) {
>
> Please use continue to avoid nesting, for example:
>
>> +      ObjCMethodDecl *SuperMethodDecl =
>> +        SD->lookupMethod(Method->getSelector(), IsInstance);
>         if (SuperMethodDecl == 0) continue;
>
>> +        ObjCMethodDecl::param_iterator ParamI = Method- 
>> >param_begin(),
>> +        E = Method->param_end();
>> +        ObjCMethodDecl::param_iterator PrevI =
>> +        SuperMethodDecl->param_begin();
>
> Do you check that the number of parameters is the same, or is that  
> guaranteed by the selector match?  Either way, please add a comment.
Guaranteed by selector match.

>
>
>> +        for (; ParamI != E; ++ParamI, ++PrevI) {
>> +          assert(PrevI != SuperMethodDecl->param_end() && "Param  
>> mismatch");
>> +          QualType T1 = Context.getCanonicalType((*ParamI)- 
>> >getType());
>> +          QualType T2 = Context.getCanonicalType((*PrevI)- 
>> >getType());
>> +          if (T1 != T2) {
>
> Likewise: if (T1 == T2) continue;  it would also be nice to add  
> comments explaining what these checks are doing.
>
>
>> +            AssignConvertType ConvTy =  
>> CheckAssignmentConstraints(T1, T2);
>> +            if (ConvTy == Incompatible || ConvTy ==  
>> IncompatiblePointer) {
>
> Here too. :)
>
> Is CheckAssignmentConstraints the right predicate here, or is  
> typesAreCompatible the right predicate?  It seems that short/int  
> should cause the warning to come out even though they are assignable.

I wasn't planning to warn other than those which are initialization  
compatible. But, as long as we are going to issue warning
we might as well be very generous. Just have to wait and see how our  
users respond. Changed to use typesAreCompatible.

In http://llvm.org/viewvc/llvm-project?view=rev&revision=78075

- Fariborz



>
>
>> @@ -1509,6 +1540,7 @@
>>        InsMap[Method->getSelector()] = Method;
>>        /// The following allows us to typecheck messages to "id".
>>        AddInstanceMethodToGlobalPool(Method);
>> +        CompareMethodParamsInBaseAndSuper(ClassDecl, Method, true);
>
> Please add a comment above this line, something along the lines of  
> "verify that the method conforms to the same definition of parent  
> methods if it shadows one." or something.
>
>>      }
>>    }
>>    else {
>> @@ -1526,6 +1558,7 @@
>>        ClsMap[Method->getSelector()] = Method;
>>        /// The following allows us to typecheck messages to "Class".
>>        AddFactoryMethodToGlobalPool(Method);
>> +        CompareMethodParamsInBaseAndSuper(ClassDecl, Method, false);
>>      }
>>    }
>>  }
>>
>> Added: cfe/trunk/test/SemaObjC/warn-superclass-method-mismatch.m
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/warn-superclass-method-mismatch.m?rev=78029&view=auto
>>
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> = 
>> =====================================================================
>> --- cfe/trunk/test/SemaObjC/warn-superclass-method-mismatch.m (added)
>> +++ cfe/trunk/test/SemaObjC/warn-superclass-method-mismatch.m Mon  
>> Aug  3 20:07:16 2009
>> @@ -0,0 +1,46 @@
>> +// RUN: clang-cc  -fsyntax-only -verify %s
>> +
>> + at interface Root
>> +-(void) method_r: (char)ch : (float*)f1 : (int*) x; // expected- 
>> note {{previous declaration is here}}
>> + at end
>> +
>> + at class Sub;
>> +
>> + at interface Base : Root
>> +-(void) method: (int*) x; // expected-note {{previous declaration  
>> is here}}
>> +-(void) method1: (Base*) x; // expected-note {{previous  
>> declaration is here}}
>> +-(void) method2: (Sub*) x;
>> ++ method3: (int)x1 : (Base *)x2 : (float)x3; // expected-note  
>> {{previous declaration is here}}
>> ++ mathod4: (id)x1;
>> + at end
>> +
>> +struct A {
>> +  int x,y,z;
>> +};
>> +
>> + at interface Sub : Base
>> +-(void) method: (struct A*) a;	// expected-warning {{method  
>> parameter type 'struct A *' does not match super class method  
>> parameter type 'int *'}}
>> +-(void) method1: (Sub*) x;	// expected-warning {{method parameter  
>> type 'Sub *' does not match super class method parameter type 'Base  
>> *'}}
>> +-(void) method2: (Base*) x;	// no need to warn. At call point we  
>> warn if need be.
>> ++ method3: (int)x1 : (Sub *)x2 : (float)x3;	// expected-warning  
>> {{method parameter type 'Sub *' does not match super class method  
>> parameter type 'Base *'}}
>> ++ mathod4: (Base*)x1;
>> +-(void) method_r: (char)ch : (float*)f1 : (Sub*) x; // expected- 
>> warning {{method parameter type 'Sub *' does not match super class  
>> method parameter type 'int *'}}
>> + at end
>> +
>> +void f(Base *base, Sub *sub) {
>> +  int x;
>> +  [base method:&x];  // warn. if base is actually 'Sub' it will  
>> use -[Sub method] with wrong arguments
>> +
>> +  Base *b;
>> +  [base method1:b]; // if base is actuall 'Sub'  it will use [Sub  
>> method1] with wrong argument.
>> +
>> +  [base method2:b];  // expected-warning {{}}
>> +
>> +  Sub *s;
>> +  [base method2:s]; // if base is actually 'Sub' OK. Either way OK.
>> +
>> +}
>> +
>> +
>> +
>> +
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>