<div dir="ltr">If you like I can also go through and point out all the places where I *removed* copies.</div><br><div class="gmail_quote"><div dir="ltr">On Wed, Sep 21, 2016 at 5:14 PM Zachary Turner <<a href="mailto:zturner@google.com">zturner@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div dir="ltr" class="gmail_msg">On Wed, Sep 21, 2016 at 5:00 PM Greg Clayton <<a href="mailto:gclayton@apple.com" class="gmail_msg" target="_blank">gclayton@apple.com</a>> wrote:<br class="gmail_msg"></div><blockquote class="gmail_quote gmail_msg" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Please submit a change requests when doing these kinds of things and let people comment on the changes before committing such things.<br class="gmail_msg">
<br class="gmail_msg">
We deleted functions that were correctly using "const char *" like:<br class="gmail_msg">
<br class="gmail_msg">
  bool Execute(llvm::StringRef string, Match *match = nullptr) const;<br class="gmail_msg">
  bool Execute(const char *, Match * = nullptr) = delete;<br class="gmail_msg">
<br class="gmail_msg">
Yet inside these functions you do "string.str().c_str()" because these need to be null terminated? this indicates that StringRef is NOT the correct choice here. You will make heap copy of all strings (if they are long enough and regex matching string can be very long) that you compile or execute. We already make a copy of the string when we compile, so StringRef didn't really save us anything on compile and execute is always making a copy just to run the expression on. I am fine with both being there in case one might be more efficient, but taking them out just to use a less efficient version that uses llvm::StringRef is not the kind of changes we should be doing all over.<br class="gmail_msg">
<br class="gmail_msg">
We will make copies of all strings in all of the following changes:<br class="gmail_msg">
<br class="gmail_msg">
- Unneeded copy:<br class="gmail_msg">
<br class="gmail_msg">
-        new TypeNameSpecifierImpl(regex->GetText(), true));<br class="gmail_msg">
+        new TypeNameSpecifierImpl(regex->GetText().str().c_str(), true));<br class="gmail_msg"></blockquote></div></div><div dir="ltr" class="gmail_msg"><div class="gmail_quote gmail_msg"><div class="gmail_msg">All of these copies are only temporary.  As I've said over and over, once everything is StringRef all the way down, the copies all disappear.  It's only a copy because TypeNameSpecifierImpl doesn't take a STringRef, and I didn't want to change the whole codebase all at once, but rather incrementally.</div></div></div></blockquote></div>