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

[Jason Haslam]

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.


> Please add serialization/deserialization support for this new field.

Done in the attached patch.


> It would be nice if there were some way to test for this source location information, but unfortunately I can't think of one. The fact that you'll be using it in your IDE will have to suffice.

Thanks!

Jason

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