[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

[Chris Lattner]

On Oct 4, 2007, at 11:37 AM, Steve Naroff wrote:
> 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.

Ah, I see the inspiration.  The difference here is that  
Type::getAsStringInternal needs a buffer to mutate internally before  
it returns it.  It is adding prefixes and suffixes onto it.  You're  
right that it should have a form that returns an std::string for use  
in diagnostics.

> 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.

Hey, I totally agree with you here. Unfortunately "count" is an idiom  
that comes from the STL.  :(  I assume that it is for generality  
across multimap and multisets where a key can be in the container  
more than once.

> 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:-)

The getName() expression wouldn't be as efficient?

-Chris