[cfe-commits] r42412 - in /cfe/trunk: AST/Decl.cpp Parse/ParseObjc.cpp Parse/Parser.cpp Sema/Sema.h Sema/SemaDecl.cpp clang.xcodeproj/project.pbxproj include/clang/AST/Decl.h include/clang/Basic/DiagnosticKinds.def include/clang/Parse/Action.h include/clang/Parse/Parser.h test/Sema/method-undefined-warn-1.m

[Steve Naroff]

On Oct 4, 2007, at 11:01 AM, Chris Lattner wrote:

> On Sep 27, 2007, at 11:57 AM, Fariborz Jahanian wrote:
>> +  for (int j = 0; j < IDecl->getNumInsMethods(); j++)
>> +    if (!Map.count(methods[j]->getSelector())) {
>> +      llvm::SmallString<128> buf;
>> +      Diag(methods[j]->getLocation(), diag::warn_undef_method_impl,
>> +           methods[j]->getSelector()->getName(buf));
>> +    }
>
> This seems awkward: I think it would make sense to add a getNameAsStr
> () method which returns an std::string directly (or have a getName()
> which takes no argument).  With the current code, an std::string is
> being transparently constructed anyway, so there should be no perf
> penalty.
>

I don't understand why this is awkward, since this is similar to  
Type::getAsStringInternal(buf). Aren't they solving the same problem?  
curious.

btw...what I find more awkward about the code snippit is...

>> +    if (!Map.count(methods[j]->getSelector())) {

Since I am reading the code in context, I can imagine that "count" is  
being used as a "lookup" function.

Nevertheless, in isolation, the preceding expression doesn't read  
well or make sense (I discussed this with Fariborz when I reviewed  
his patch...he said Chris said this was the most efficient:-)

snaroff

>> @@ -1560,7 +1603,17 @@
>>                                          static_cast<Decl*>
>> (ClassDecl));
>>      Category->ObjcAddCatMethods(&insMethods[0], insMethods.size(),
>>                                  &clsMethods[0], clsMethods.size());
>> -  }
>> +  }
>> +  else if (isa<ObjcImplementationDecl>(static_cast<Decl *>
>> (ClassDecl))) {
>> +    ObjcImplementationDecl* ImplClass =  
>> cast<ObjcImplementationDecl>(
>> +                                               static_cast<Decl*>
>> (ClassDecl));
>
> Fariborz, this should use dyn_cast instead of isa + cast.
>
> -Chris
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits