[cfe-commits] r124609 - in /cfe/trunk: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaObjCProperty.cpp test/SemaObjC/custom-atomic-property.m

[jahanian]

On Jan 31, 2011, at 1:34 PM, Argyrios Kyrtzidis wrote:

> Author: akirtzidis
> Date: Mon Jan 31 15:34:11 2011
> New Revision: 124609
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=124609&view=rev
> Log:
> Add -Wcustom-atomic-properties which warns if an atomic-by-default property has custom getter or setter.
> 
> The rationale is that it is highly likely that the user's getter/setter isn't atomically implemented. Off by default.
> Addresses rdar://8782645.
> 
> -Wcustom-atomic-properties and -Wimplicit-atomic-properties are under the -Watomic-properties group.
> 
> Added:
>    cfe/trunk/test/SemaObjC/custom-atomic-property.m
> Modified:
>    cfe/trunk/include/clang/Basic/DiagnosticGroups.td
>    cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
>    cfe/trunk/lib/Sema/SemaObjCProperty.cpp
> 
> Modified: cfe/trunk/include/clang/Basic/DiagnosticGroups.td
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticGroups.td?rev=124609&r1=124608&r2=124609&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticGroups.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticGroups.td Mon Jan 31 15:34:11 2011
> @@ -151,6 +151,9 @@
> def Reorder : DiagGroup<"reorder">;
> def UndeclaredSelector : DiagGroup<"undeclared-selector">;
> def ImplicitAtomic : DiagGroup<"implicit-atomic-properties">;
> +def CustomAtomic : DiagGroup<"custom-atomic-properties">;
> +def AtomicProperties : DiagGroup<"atomic-properties",
> +                                 [ImplicitAtomic, CustomAtomic]>;
> def Selector : DiagGroup<"selector">;
> def NonfragileAbi2 : DiagGroup<"nonfragile-abi2">;
> def Protocol : DiagGroup<"protocol">;
> 
> Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=124609&r1=124608&r2=124609&view=diff
> ==============================================================================
> --- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
> +++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Jan 31 15:34:11 2011
> @@ -386,6 +386,10 @@
> def warn_atomic_property_rule : Warning<
>   "writable atomic property %0 cannot pair a synthesized setter/getter "
>   "with a user defined setter/getter">;
> +def warn_default_atomic_custom_getter_setter : Warning<
> +  "atomic by default property %0 has a user defined setter/getter "
> +  "(property should be marked 'atomic' if this is intended)">,
> +  InGroup<CustomAtomic>, DefaultIgnore;
> def err_use_continuation_class : Error<
>   "illegal redeclaration of property in continuation class %0"
>   " (attribute must be 'readwrite', while its primary must be 'readonly')">;
> 
> Modified: cfe/trunk/lib/Sema/SemaObjCProperty.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaObjCProperty.cpp?rev=124609&r1=124608&r2=124609&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Sema/SemaObjCProperty.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaObjCProperty.cpp Mon Jan 31 15:34:11 2011
> @@ -1081,7 +1081,32 @@
>        E = IDecl->prop_end();
>        I != E; ++I) {
>     ObjCPropertyDecl *Property = (*I);
> +    ObjCMethodDecl *GetterMethod = 0;
> +    ObjCMethodDecl *SetterMethod = 0;
> +    bool LookedUpGetterSetter = false;
> +
>     unsigned Attributes = Property->getPropertyAttributes();
> +    unsigned AttributesAsWrittern = Property->getPropertyAttributesAsWritten();
> +
> +    if (!(AttributesAsWrittern & ObjCPropertyDecl::OBJC_PR_atomic) &&
> +        !(AttributesAsWrittern & ObjCPropertyDecl::OBJC_PR_nonatomic)) {
> +      GetterMethod = IMPDecl->getInstanceMethod(Property->getGetterName());
> +      SetterMethod = IMPDecl->getInstanceMethod(Property->getSetterName());
> +      LookedUpGetterSetter = true;
> +      if (GetterMethod) {
> +        Diag(GetterMethod->getLocation(),
> +             diag::warn_default_atomic_custom_getter_setter)
> +          << Property->getIdentifier();
> +        Diag(Property->getLocation(), diag::note_property_declare);
> +      }
> +      if (SetterMethod) {
> +        Diag(SetterMethod->getLocation(),
> +             diag::warn_default_atomic_custom_getter_setter)
> +          << Property->getIdentifier();
> +        Diag(Property->getLocation(), diag::note_property_declare);
> +      }
> +    }
> +
>     // We only care about readwrite atomic property.
>     if ((Attributes & ObjCPropertyDecl::OBJC_PR_nonatomic) ||
>         !(Attributes & ObjCPropertyDecl::OBJC_PR_readwrite))
> @@ -1090,10 +1115,11 @@
>          = IMPDecl->FindPropertyImplDecl(Property->getIdentifier())) {
>       if (PIDecl->getPropertyImplementation() == ObjCPropertyImplDecl::Dynamic)
>         continue;

Why don't you warn if property is dynamic?  They still have custom setter/getter.
> -      ObjCMethodDecl *GetterMethod =
> -        IMPDecl->getInstanceMethod(Property->getGetterName());
> -      ObjCMethodDecl *SetterMethod =
> -        IMPDecl->getInstanceMethod(Property->getSetterName());
> +      if (!LookedUpGetterSetter) {
> +        GetterMethod = IMPDecl->getInstanceMethod(Property->getGetterName());
> +        SetterMethod = IMPDecl->getInstanceMethod(Property->getSetterName());
> +        LookedUpGetterSetter = true;
> +      }
>       if ((GetterMethod && !SetterMethod) || (!GetterMethod && SetterMethod)) {
>         SourceLocation MethodLoc =
>           (GetterMethod ? GetterMethod->getLocation()

I think you don't want to warn if getter/setter are synthesized. Does this handle it?

> 
> Added: cfe/trunk/test/SemaObjC/custom-atomic-property.m
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaObjC/custom-atomic-property.m?rev=124609&view=auto
> ==============================================================================
> --- cfe/trunk/test/SemaObjC/custom-atomic-property.m (added)
> +++ cfe/trunk/test/SemaObjC/custom-atomic-property.m Mon Jan 31 15:34:11 2011
> @@ -0,0 +1,10 @@
> +// RUN: %clang_cc1  -fsyntax-only -Wcustom-atomic-properties -verify %s
> +
> + at interface Foo
> + at property (assign) Foo *myProp; // expected-note {{property declared here}} expected-note {{property declared here}}
> + at end
> +
> + at implementation Foo
> + -(Foo*)myProp {return 0;} // expected-warning {{atomic by default property 'myProp' has a user defined setter/getter (property should be marked 'atomic' if this is intended)}}
> + -(void)setMyProp:(Foo*)e {} // expected-warning {{atomic by default property 'myProp' has a user defined setter/getter (property should be marked 'atomic' if this is intended)}}

It is better for diagnostic to say setter for setters and getter for getters, instead of setter/getter for both.

- Fariborz

> + at end
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits