<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Oct 4, 2007, at 3:01 PM, Steve Naroff wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="word-wrap: break-word; -khtml-nbsp-mode: space; -khtml-line-break: after-white-space; "><br><div><div>On Oct 4, 2007, at 2:48 PM, Chris Lattner wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">On Oct 4, 2007, at 11:37 AM, Steve Naroff wrote:</div> <blockquote type="cite"><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">On Oct 4, 2007, at 11:01 AM, Chris Lattner wrote:</div> <blockquote type="cite"><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">On Sep 27, 2007, at 11:57 AM, Fariborz Jahanian wrote:</div> <blockquote type="cite"><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">+<span class="Apple-converted-space"> </span>for (int j = 0; j < IDecl->getNumInsMethods(); j++)</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">+<span class="Apple-converted-space"> </span>if (!Map.count(methods[j]->getSelector())) {</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">+<span class="Apple-converted-space"> </span>llvm::SmallString<128> buf;</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">+<span class="Apple-converted-space"> </span>Diag(methods[j]->getLocation(), diag::warn_undef_method_impl,</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">+ <span class="Apple-converted-space"> </span>methods[j]->getSelector()->getName(buf));</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">+<span class="Apple-converted-space"> </span>}</div> </blockquote><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; min-height: 14px; "><br></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">This seems awkward: I think it would make sense to add a getNameAsStr</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">() method which returns an std::string directly (or have a getName()</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">which takes no argument).<span class="Apple-converted-space"> </span>With the current code, an std::string is</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">being transparently constructed anyway, so there should be no perf</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">penalty.</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; min-height: 14px; "><br></div> </blockquote><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; min-height: 14px; "><br></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">I don't understand why this is awkward, since this is similar to Type::getAsStringInternal(buf). Aren't they solving the same problem? curious.</div> </blockquote><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; min-height: 14px; "><br></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">Ah, I see the inspiration.<span class="Apple-converted-space"> </span>The difference here is that Type::getAsStringInternal needs a buffer to mutate internally before it returns it.<span class="Apple-converted-space"> </span>It is adding prefixes and suffixes onto it.<span class="Apple-converted-space"> </span>You're right that it should have a form that returns an std::string for use in diagnostics.</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; min-height: 14px; "><br></div> <blockquote type="cite"><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">btw...what I find more awkward about the code snippit is...</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; min-height: 14px; "><br></div> <blockquote type="cite"><blockquote type="cite"><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">+<span class="Apple-converted-space"> </span>if (!Map.count(methods[j]->getSelector())) {</div> </blockquote></blockquote><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; min-height: 14px; "><br></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">Since I am reading the code in context, I can imagine that "count" is being used as a "lookup" function.</div> </blockquote><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; min-height: 14px; "><br></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">Hey, I totally agree with you here. Unfortunately "count" is an idiom that comes from the STL.<span class="Apple-converted-space"> </span>:(<span class="Apple-converted-space"> </span>I assume that it is for generality across multimap and multisets where a key can be in the container more than once.</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; min-height: 14px; "><br></div> <blockquote type="cite"><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">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:-)</div> </blockquote><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; min-height: 14px; "><br></div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">The getName() expression wouldn't be as efficient?</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; min-height: 14px; "><br></div></blockquote><div><br class="khtml-block-placeholder"></div>My previous comment applied to the Map.count idiom (not getName()). I have no problem with the efficiency of getName()...obviously:-) </div><div><br class="khtml-block-placeholder"></div><div>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...</div></div></blockquote><div><br class="webkit-block-placeholder"></div>Since we have getName() for Identifiers, it makes sense to have one for selectors with a buyer-beware clause added to it :).</div><div><br class="webkit-block-placeholder"></div><div>- Fariborz</div><div><br><blockquote type="cite"><div style="word-wrap: break-word; -khtml-nbsp-mode: space; -khtml-line-break: after-white-space; "><div><br class="khtml-block-placeholder"></div><div>snaroff</div><div><br class="khtml-block-placeholder"></div><div><blockquote type="cite"><font class="Apple-style-span" color="#000000"><br class="khtml-block-placeholder"></font><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; ">-Chris</div><div style="margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px; min-height: 14px; "><br></div> </blockquote></div><br></div></blockquote></div><br></body></html>