<br><br><div class="gmail_quote">On Wed, Aug 24, 2011 at 3:38 PM, Jordy Rose <span dir="ltr"><<a href="mailto:jediknil@belkadan.com" target="_blank">jediknil@belkadan.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
A couple comments, though this certainly isn't an area of the code I'm that familiar with.<br></blockquote><div><br></div><div>Thanks for looking</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- Instead of appendTo(string&) and assignTo(string&), why not just add operator+=(string&, const Twine&) and operator=(string&, const Twine&) ? Seems more C++ to me.<br></blockquote><div><br></div>
<div>You're right about appendTo, op+=(std::string&, const Twine&) could just be a friend function of Twine. I'll make that change.<br>Unfortunately op=(std::string&, const Twine&) can't be done because op= must be a non-static member function (of std::string). This is why I hadn't done appendTo as op+= too - I'd assumed it had the same restriction, though that doesn't appear to be the case.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">- Re: toNullTerminatedStringRef: A StringRef created from a null-terminated C string drops the null terminator, so you can't just "test the last character" to see if it's a null. In fact, having the last character of a StringRef be null is probably a bug. (Of course, you can't test /past/ the last character either, because one byte past valid memory is guaranteed to be a valid address but not guaranteed to be dereferenceable.)<br>
</blockquote><div><br></div><div>Agreed - pity, though. [I wonder if we could squeeze in a bit (the high bit of the length?) somewhere to store "is this null terminated" - it seems a pity to lose that so often/so easily when going into the StringRef domain]</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">- You've got several copies of SafeBool.h in the file. I'm guessing this is the result of reverting and then reapplying patches. (I do this all the time too.)<br>
</blockquote><div><br></div><div>Hrm, thanks for that. I'll make a fresh diff. I've done that & manually inspected the diff file & I only see SafeBool.h listed once now.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- TwineString definitely seems evil, but I haven't really thought about it hard enough to give a good reason why.</blockquote>
<div><br></div><div>Oh, it is rather evil, just a moderately quick & dirty, but not utterly broken, solution. The most concrete reason I can come up with is that it muddies StringRef's semantics, mostly - TwineString is a StringRef, but it doesn't at all have the semantics of a StringRef, in fact it has the semantics of a string (mostly... some of the time... if it's not just actually a StringRef)</div>
<div><br></div><div>To quote Chris from a previous email where this was discussed:<br><br>"<span style="font-family:monospace;white-space:pre-wrap;background-color:rgb(255, 255, 255);font-size:medium">While it is kinda gross, a subclass of StringRef is probably the lowest friction path to do this." - </span><a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2011-July/041804.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvmdev/2011-July/041804.html</a></div>
<div><br></div><div>Not to say that I'm not open to other ideas...</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> I don't get why you're using a SmallVectorImpl instead of a SmallVector or SmallString, though.<br>
</blockquote><div><br></div><div>I think I just used SmallVectorImpl because it's the type that toStringRef required - but SmallString would make more sense if it's got no additional overhead/quirks (or SmallVector, presumably it really doesn't have extra overhead).<br>
Settled on SmallString.</div>
<div><br></div><div>- David</div><div><br></div></div>