<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Dec 8, 2011, at 12:55 PM, Jason Haslam wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Dec 8, 2011, at 1:10 PM, Douglas Gregor wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Dec 8, 2011, at 11:57 AM, Jason Haslam wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Dec 8, 2011, at 12:27 PM, jahanian wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div><div><div>On Dec 8, 2011, at 11:14 AM, Jason Haslam wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>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.<br></div></blockquote>We have couple of locations you may use:<div><br></div><div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; ">ObjCCategoryImplDecl(<span style="color: rgb(86, 129, 134); ">DeclContext</span> *DC, <span style="color: rgb(86, 129, 134); ">IdentifierInfo</span> *Id,</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "> <span style="color: rgb(86, 129, 134); ">ObjCInterfaceDecl</span> *classInterface,</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; font: normal normal normal 11px/normal Menlo; "> <span style="color: rgb(86, 129, 134); ">SourceLocation</span> nameLoc, <span style="color: rgb(86, 129, 134); ">SourceLocation</span> atStartLoc)</div><div><br></div></div></div></div></div></div></blockquote><div><br></div><div>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.</div></div></div></blockquote><br></div><div>In this case, I disagree with Fariborz.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>I do have a few specific comments on the patch:</div><div><br></div><div><div>@@ -1310,6 +1315,9 @@</div><div> </div><div> ObjCCategoryDecl *getCategoryDecl() const;</div><div> </div><div>+ SourceLocation getCategoryNameLoc() const { return CategoryNameLoc; }</div><div>+ void setCategoryNameLoc(SourceLocation Loc) { CategoryNameLoc = Loc; }</div><div>+</div><div> /// getName - Get the name of identifier for the class interface associated</div><div> /// with this implementation as a StringRef.</div><div> //</div><div><br></div><div>We don't really need the setter here, do we?</div></div></div></blockquote><div><br></div><div>It's used now in deserialization.</div></div></div></blockquote><div><br></div><div>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.</div><div>Please remove the setter and access the field directly in deserialization.</div><div><br></div><div>BTW, thanks for doing this, it was on my todo list :-).</div><div><br></div><div>-Argyrios</div><div><br></div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div><br></div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>Please add serialization/deserialization support for this new field.</div></div></div></blockquote><div><br></div><div>Done in the attached patch.</div><div><br></div><br><blockquote type="cite"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>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.</div></div></div></blockquote><br></div><div>Thanks!</div><div><br></div><div>Jason</div><div><br></div><div></div></div><span><category_impl_name_loc.diff></span><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div></div></div>_______________________________________________<br>cfe-commits mailing list<br><a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits<br></blockquote></div><br></body></html>