[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

[Chris Lattner]

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.

> +        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.

> @@ -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