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

Douglas Gregor dgregor at apple.com
Thu Dec 8 12:10:02 PST 2011


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?

Please add serialization/deserialization support for this new field.

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.

	- Doug

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


More information about the cfe-commits mailing list