[cfe-commits] r157435 - in /cfe/trunk: include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaExprObjC.cpp lib/Sema/SemaPseudoObject.cpp test/SemaObjC/property-user-setter.m

[Ted Kremenek]

Thanks for doing this.  Comments inline.

On May 24, 2012, at 3:48 PM, Fariborz Jahanian <fjahanian at apple.com> wrote:

> Author: fjahanian
> Date: Thu May 24 17:48:38 2012
> New Revision: 157435
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=157435&view=rev
> Log:
> objective-c: warn on use of property setters
> backing two propeties because proprty names
> match except for first letter being of different
> case. // rdar://11528439, [PR12936].
> 
> Modified:

<SNIP>

> ///
> /// \return true if a setter was found, in which case Setter 
> -bool ObjCPropertyOpBuilder::findSetter() {
> +bool ObjCPropertyOpBuilder::findSetter(bool warn) {
>   // For implicit properties, just trust the lookup we already did.
>   if (RefExpr->isImplicitProperty()) {
>     if (ObjCMethodDecl *setter = RefExpr->getImplicitPropertySetter()) {
> @@ -531,6 +532,23 @@
>   // Do a normal method lookup first.
>   if (ObjCMethodDecl *setter =
>         LookupMethodInReceiverType(S, SetterSelector, RefExpr)) {
> +    if (setter->isSynthesized() && warn)

It's probably better to flip this, since 'warn' is a simple variable the setter->isSynthesized() is a function call.

> +      if (const ObjCInterfaceDecl *IFace =
> +          dyn_cast<ObjCInterfaceDecl>(setter->getDeclContext())) {
> +        const StringRef thisPropertyName(prop->getName());
> +        char front = thisPropertyName.front();
> +        front = islower(front) ? toupper(front) : tolower(front);
> +        SmallString<100> PropertyName = thisPropertyName;
> +        PropertyName[0] = front;
> +        IdentifierInfo *AltMember = &S.PP.getIdentifierTable().get(PropertyName);
> +        if (ObjCPropertyDecl *prop1 = IFace->FindPropertyDeclaration(AltMember))
> +          if (prop != prop1 && (prop1->getSetterMethodDecl() == setter)) {
> +            S.Diag(RefExpr->getExprLoc(), diag::warn_property_setter_ambiguous_use)
> +              << prop->getName() << prop1->getName() << setter->getSelector();
> +            S.Diag(prop->getLocation(), diag::note_property_declare);
> +            S.Diag(prop1->getLocation(), diag::note_property_declare);
> +          }
> +      }

I'm wondering if we should just make this a hard error.  What is the expected semantics of code that declares two properties whose accessors collide, especially when these are synthesized properties?  Do we synthesize both accessors (not certain what that would mean)?  Do we essentially drop one of the properties?  The logic here looks like it's trying to find a naming conflict, and issue a soft warning, but the code is essentially broken.  It's as if the same methods we're declared twice within the same @interface, which makes no sense at all.


>     Setter = setter;
>     return true;
>   }
> @@ -603,7 +621,7 @@
> ///   value being set as the value of the property operation.
> ExprResult ObjCPropertyOpBuilder::buildSet(Expr *op, SourceLocation opcLoc,
>                                            bool captureSetValueAsResult) {
> -  bool hasSetter = findSetter();
> +  bool hasSetter = findSetter(false);
>   assert(hasSetter); (void) hasSetter;
> 
>   if (SyntacticRefExpr)
> 
> Modified: cfe/trunk/test/SemaObjC/property-user-setter.m
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/property-user-setter.m?rev=157435&r1=157434&r2=157435&view=diff
> ==============================================================================
> --- cfe/trunk/test/SemaObjC/property-user-setter.m (original)
> +++ cfe/trunk/test/SemaObjC/property-user-setter.m Thu May 24 17:48:38 2012
> @@ -111,8 +111,10 @@
> @property (copy) id p;
> @property (copy) id r;
> @property (copy) id Q;
> - at property (copy) id t;
> - at property (copy) id T;
> + at property (copy) id t; // expected-note 2 {{property declared here}}
> + at property (copy) id T; // expected-note 2 {{property declared here}}
> + at property (copy) id Pxyz; // expected-note 2 {{property declared here}}
> + at property (copy) id pxyz; // expected-note 2 {{property declared here}}
> @end
> 
> @implementation rdar11363363
> @@ -120,11 +122,15 @@
> @synthesize r;
> @synthesize Q;
> @synthesize t, T;
> + at synthesize Pxyz, pxyz;
> - (id) Meth {
>   self.P = 0; // expected-error {{property 'P' not found on object of type 'rdar11363363 *'}}
>   self.q = 0; // expected-error {{property 'q' not found on object of type 'rdar11363363 *'}}
> -  self.t = 0; // OK
> -  self.T = 0; // OK
> +// rdar://11528439
> +  self.t = 0; // expected-warning {{synthesized properties 't' and 'T' both claim setter 'setT:'}}
> +  self.T = 0; // expected-warning {{synthesized properties 'T' and 't' both claim setter 'setT:'}}
> +  self.Pxyz = 0; // expected-warning {{synthesized properties 'Pxyz' and 'pxyz' both claim setter 'setPxyz:'}}
> +  self.pxyz = 0; // expected-warning {{synthesized properties 'pxyz' and 'Pxyz' both claim setter 'setPxyz:'}}
>   self.R = 0; // expected-error {{property 'R' not found on object of type 'rdar11363363 *'; did you mean to access ivar 'R'?}}
>   return self.R; // expected-error {{property 'R' not found on object of type 'rdar11363363 *'; did you mean to access ivar 'R'?}}
> }
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits