Only complain about __strong __strong id, not __strong SomeStrongTypedef

or __strong __typeof__(some.strong.thing).

llvm-svn: 150029

4 files changed, 71 insertions, 17 deletions

diff --git a/clang/lib/Sema/SemaType.cpp b/clang/lib/Sema/SemaType.cpp
index 67ee9376e8b..daf0b683b64 100644
--- a/clang/lib/Sema/SemaType.cpp
+++ b/clang/lib/Sema/SemaType.cpp
@@ -3236,6 +3236,36 @@ static void HandleAddressSpaceTypeAttribute(QualType &Type,
   Type = S.Context.getAddrSpaceQualType(Type, ASIdx);
 }
 
+/// Does this type have a "direct" ownership qualifier?  That is,
+/// is it written like "__strong id", as opposed to something like
+/// "typeof(foo)", where that happens to be strong?
+static bool hasDirectOwnershipQualifier(QualType type) {
+  // Fast path: no qualifier at all.
+  assert(type.getQualifiers().hasObjCLifetime());
+
+  while (true) {
+    // __strong id
+    if (const AttributedType *attr = dyn_cast<AttributedType>(type)) {
+      if (attr->getAttrKind() == AttributedType::attr_objc_ownership)
+        return true;
+
+      type = attr->getModifiedType();
+
+    // X *__strong (...)
+    } else if (const ParenType *paren = dyn_cast<ParenType>(type)) {
+      type = paren->getInnerType();
+   
+    // That's it for things we want to complain about.  In particular,
+    // we do not want to look through typedefs, typeof(expr),
+    // typeof(type), or any other way that the type is somehow
+    // abstracted.
+    } else {
+      
+      return false;
+    }
+  }
+}
+
 /// handleObjCOwnershipTypeAttr - Process an objc_ownership
 /// attribute on the specified type.
 ///
@@ -3264,12 +3294,6 @@ static bool handleObjCOwnershipTypeAttr(TypeProcessingState &state,
   if (AttrLoc.isMacroID())
     AttrLoc = S.getSourceManager().getImmediateExpansionRange(AttrLoc).first;
 
-  if (type.getQualifiers().getObjCLifetime()) {
-    S.Diag(AttrLoc, diag::err_attr_objc_ownership_redundant)
-      << type;
-    return true;
-  }
-
   if (!attr.getParameterName()) {
     S.Diag(AttrLoc, diag::err_attribute_argument_n_not_string)
       << "objc_ownership" << 1;
@@ -3277,6 +3301,11 @@ static bool handleObjCOwnershipTypeAttr(TypeProcessingState &state,
     return true;
   }
 
+  // Consume lifetime attributes without further comment outside of
+  // ARC mode.
+  if (!S.getLangOptions().ObjCAutoRefCount)
+    return true;
+
   Qualifiers::ObjCLifetime lifetime;
   if (attr.getParameterName()->isStr("none"))
     lifetime = Qualifiers::OCL_ExplicitNone;
@@ -3293,10 +3322,31 @@ static bool handleObjCOwnershipTypeAttr(TypeProcessingState &state,
     return true;
   }
 
-  // Consume lifetime attributes without further comment outside of
-  // ARC mode.
-  if (!S.getLangOptions().ObjCAutoRefCount)
-    return true;
+  SplitQualType underlyingType = type.split();
+
+  // Check for redundant/conflicting ownership qualifiers.
+  if (Qualifiers::ObjCLifetime previousLifetime
+        = type.getQualifiers().getObjCLifetime()) {
+    // If it's written directly, that's an error.
+    if (hasDirectOwnershipQualifier(type)) {
+      S.Diag(AttrLoc, diag::err_attr_objc_ownership_redundant)
+        << type;
+      return true;
+    }
+
+    // Otherwise, if the qualifiers actually conflict, pull sugar off
+    // until we reach a type that is directly qualified.
+    if (previousLifetime != lifetime) {
+      // This should always terminate: the canonical type is
+      // qualified, so some bit of sugar must be hiding it.
+      while (!underlyingType.Quals.hasObjCLifetime()) {
+        underlyingType = underlyingType.getSingleStepDesugaredType();
+      }
+      underlyingType.Quals.removeObjCLifetime();
+    }
+  }
+
+  underlyingType.Quals.addObjCLifetime(lifetime);
 
   if (NonObjCPointer) {
     StringRef name = attr.getName()->getName();
@@ -3312,11 +3362,9 @@ static bool handleObjCOwnershipTypeAttr(TypeProcessingState &state,
       << name << type;
   }
 
-  Qualifiers qs;
-  qs.setObjCLifetime(lifetime);
   QualType origType = type;
   if (!NonObjCPointer)
-    type = S.Context.getQualifiedType(type, qs);
+    type = S.Context.getQualifiedType(underlyingType);
 
   // If we have a valid source location for the attribute, use an
   // AttributedType instead.
diff --git a/clang/test/SemaObjC/arc-objc-lifetime.m b/clang/test/SemaObjC/arc-objc-lifetime.m
index 82c63892718..ce525db7b5b 100644
--- a/clang/test/SemaObjC/arc-objc-lifetime.m
+++ b/clang/test/SemaObjC/arc-objc-lifetime.m
@@ -32,3 +32,11 @@ typedef __autoreleasing NSString * AUTORELEASEPNSString;
         __autoreleasing id *stuff = (__autoreleasing id *)addr;
 }
 @end
+
+// rdar://problem/10711456
+__strong I *__strong test1; // expected-error {{the type 'I *__strong' is already explicitly ownership-qualified}}
+__strong I *(__strong test2); // expected-error {{the type 'I *__strong' is already explicitly ownership-qualified}}
+__strong I *(__strong (test3)); // expected-error {{the type 'I *__strong' is already explicitly ownership-qualified}}
+__unsafe_unretained __typeof__(test3) test4;
+typedef __strong I *strong_I;
+__unsafe_unretained strong_I test5;
diff --git a/clang/test/SemaObjC/arc.m b/clang/test/SemaObjC/arc.m
index 7a9655037eb..d9ae5b403cc 100644
--- a/clang/test/SemaObjC/arc.m
+++ b/clang/test/SemaObjC/arc.m
@@ -39,8 +39,6 @@ void test1(A *a) {
 }
 @end
 
-__weak __strong id x; // expected-error {{the type '__strong id' already has retainment attributes}}
-
 // rdar://8843638
 
 @interface I
diff --git a/clang/test/SemaObjCXX/arc-templates.mm b/clang/test/SemaObjCXX/arc-templates.mm
index 931b21f5d47..9eca84648f6 100644
--- a/clang/test/SemaObjCXX/arc-templates.mm
+++ b/clang/test/SemaObjCXX/arc-templates.mm
@@ -97,8 +97,8 @@ int check_make_weak2[is_same<make_weak<__autoreleasing id>::type, __weak id>::va
 template<typename T>
 struct make_weak_fail {
   typedef T T_type;
-  typedef __weak T_type type; // expected-error{{the type 'T_type' (aka '__weak id') already has retainment attributes set on it}} \
-  // expected-error{{the type 'T_type' (aka '__strong id') already has retainment attributes set on it}}
+  typedef __weak T_type type; // expected-error{{the type 'T_type' (aka '__weak id') is already explicitly ownership-qualified}} \
+  // expected-error{{the type 'T_type' (aka '__strong id') is already explicitly ownership-qualified}}
 };
 
 int check_make_weak_fail0[is_same<make_weak_fail<__weak id>::type, __weak id>::value? 1 : -1]; // expected-note{{in instantiation of template class 'make_weak_fail<__weak id>' requested here}}