[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

[Fariborz Jahanian]

On Oct 4, 2007, at 3:01 PM, Steve Naroff wrote:

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

Since we have getName() for Identifiers, it makes sense to have one  
for selectors with a buyer-beware clause added to it :).

- Fariborz

>
> snaroff
>
>>
>> -Chris
>>
>

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