[cfe-commits] r163813 - in /cfe/trunk/lib/Sema: SemaDeclObjC.cpp SemaType.cpp

[jahanian]

On Sep 13, 2012, at 11:34 AM, John McCall wrote:

> On Sep 13, 2012, at 10:29 AM, Fariborz Jahanian wrote:
>> Modified: cfe/trunk/lib/Sema/SemaType.cpp
>> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaType.cpp?rev=163813&r1=163812&r2=163813&view=diff
>> ==============================================================================
>> --- cfe/trunk/lib/Sema/SemaType.cpp (original)
>> +++ cfe/trunk/lib/Sema/SemaType.cpp Thu Sep 13 12:29:07 2012
>> @@ -3335,6 +3335,36 @@
>>  return TInfo;
>> }
>> 
>> +
>> +/// checkImplicitObjCParamAttribute - diagnoses when pointer to ObjC pointer
>> +/// has implicit ownership attribute.
>> +static void
>> +checkImplicitObjCParamAttribute(Sema &S, Declarator &D, QualType T) {
>> +  if (!S.getLangOpts().ObjCAutoRefCount ||
>> +      !S.OriginalLexicalContext ||
>> +      (S.OriginalLexicalContext->getDeclKind() != Decl::ObjCImplementation &&
>> +       S.OriginalLexicalContext->getDeclKind() != Decl::ObjCCategoryImpl))
>> +    return;
> 
> In general, please use isa<> checks instead of looking at getDeclKind() directly.
> 
> I'm sorry for the run-around;  I'd forgotten the restricted scope of this warning.
> You're right that this is more appropriate to diagnose in SemaDeclObjC.
> 
>> +  if (!T->isObjCIndirectLifetimeType())
>> +    return;
> 
> You're basically doing most of the checks for this anyway;  no need to do it
> first off here.
> 
>> +  if (!T->isPointerType() && !T->isReferenceType())
>> +    return;
>> +  QualType OrigT = T;
>> +  T = T->isPointerType() 
>> +    ? T->getAs<PointerType>()->getPointeeType() 
>> +    : T->getAs<ReferenceType>()->getPointeeType();
> 
> Please simplify and re-use your queries:
> 
>  if (const PointerType *PT = T->getAs<PointerType>()) {
>    T = PT->getPointeeType();
>  } else if (const ReferenceType *RT = T->getAs<ReferenceType>()) {
>    T = RT->getPointeeType();
>  } else {
>    return;
>  }
> 
>> +  if (T->isObjCLifetimeType()) {
> 
> You don't need to check this;  this is always true if there's a local
> qualifier.
> 
>> +    // when lifetime is Qualifiers::OCL_None it means that it has
>> +    // no implicit ownership qualifier (which means it is explicit).
>> +    Qualifiers::ObjCLifetime lifetime = 
>> +    T.getLocalQualifiers().getObjCLifetime();
> 
> // If we have a lifetime qualifier, but it's local, we must have inferred it.
> if (T.getLocalQualifiers().hasObjCLifetime())

In r163824. Thanks for the simplification suggestions.
- fariborz

> 
> John.