[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 2:48 PM, Chris Lattner wrote:

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

My previous comment applied to the Map.count idiom (not getName()). I  
have no problem with the efficiency of getName()...obviously:-)

As far as getName() is concerned, I am concerned about lifetime. By  
passing a buffer in, it is clear the client owns the buffer. If  
getName() returns a string buffer, the ownership is unclear. I don't  
have a strong opinion on this...I just want to point out my (minor)  
issue/concern. Clearly, not having to pass in a buffer is more  
convenient...

snaroff

>
> -Chris
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20071004/59f37b60/attachment.html>