[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 3:06 PM, Fariborz Jahanian wrote:

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

Seems confusing to me. When using IdentifierInfo::getName(), you  
*cannot* free the result (or bad things will happen:-). However, when  
using Selector::getName() you *must* free the result.

snaroff

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

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