[cfe-commits] Patch to save category name loc in ObjCCategoryImplDecl

[Jason Haslam]

On Dec 8, 2011, at 4:42 PM, Argyrios Kyrtzidis wrote:

> On Dec 8, 2011, at 12:55 PM, Jason Haslam wrote:
> 
>> On Dec 8, 2011, at 1:10 PM, Douglas Gregor wrote:
>> 
>>> 
>>> On Dec 8, 2011, at 11:57 AM, Jason Haslam wrote:
>>> 
>>>> On Dec 8, 2011, at 12:27 PM, jahanian wrote:
>>>> 
>>>>> On Dec 8, 2011, at 11:14 AM, Jason Haslam wrote:
>>>>> 
>>>>>> Hmm… okay. I guess that the only justification that I have is that we use it in our IDE to point to the location of the category implementation (e.g. when the user right clicks on the category name in the interface declaration and selects 'Edit Implementation'). There's no other use in clang itself.
>>>>> We have couple of locations you may use:
>>>>> 
>>>>> ObjCCategoryImplDecl(DeclContext *DC, IdentifierInfo *Id,
>>>>>                        ObjCInterfaceDecl *classInterface,
>>>>>                        SourceLocation nameLoc, SourceLocation atStartLoc)
>>>>> 
>>>> 
>>>> Yes, I noticed those. We like to select the name of the referenced entity when we jump to its location. And deriving the location of the category name from the location of the class name seemed too error prone and complicated, especially given that it's already known at the point where the node is constructed. However, this is really too minor a point to argue about. If you don't feel that it adds any value that's good enough for me. Thanks for the review.
>>> 
>>> In this case, I disagree with Fariborz.
>>> 
>>> We should be tracking this information, because we should know about all of the parsed identifiers in the source code. Moreover, if we some day want to support renaming of categories, we'd need this location information to do it.
>>> 
>>> I am not at all concerned about AST size for this particular case, since it's extremely rare for there to be more than a tiny number of @implementation directives of any kind within a single translation unit.
>>> 
>>> I do have a few specific comments on the patch:
>>> 
>>> @@ -1310,6 +1315,9 @@
>>>  
>>>    ObjCCategoryDecl *getCategoryDecl() const;
>>>  
>>> +  SourceLocation getCategoryNameLoc() const { return CategoryNameLoc; }
>>> +  void setCategoryNameLoc(SourceLocation Loc) { CategoryNameLoc = Loc; }
>>> +
>>>    /// getName - Get the name of identifier for the class interface associated
>>>    /// with this implementation as a StringRef.
>>>    //
>>> 
>>> We don't really need the setter here, do we?
>> 
>> It's used now in deserialization.
> 
> Readers/Writers generally have access to private fields, and we prefer to set private fields directly instead of using methods, because deserialization is really easy to break invariants of methods and it required internal knowledge.
> Please remove the setter and access the field directly in deserialization.
> 
> BTW, thanks for doing this, it was on my todo list :-).

Okay, makes sense. Done in the attached patch.

Jason



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111208/2a4e944c/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: category_impl_name_loc.diff
Type: application/octet-stream
Size: 5880 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111208/2a4e944c/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111208/2a4e944c/attachment-0001.html>