> On Sep 21, 2016, at 5:14 PM, Zachary Turner <ztur...@google.com> wrote: > > > > On Wed, Sep 21, 2016 at 5:00 PM Greg Clayton <gclay...@apple.com> wrote: > Please submit a change requests when doing these kinds of things and let > people comment on the changes before committing such things. > > We deleted functions that were correctly using "const char *" like: > > bool Execute(llvm::StringRef string, Match *match = nullptr) const; > bool Execute(const char *, Match * = nullptr) = delete; > > 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. > > We will make copies of all strings in all of the following changes: > > - Unneeded copy: > > - new TypeNameSpecifierImpl(regex->GetText(), true)); > + new TypeNameSpecifierImpl(regex->GetText().str().c_str(), true)); > 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.
As part of these changes we absolutely should add StringRef to these classes and we should just make it part of the changes. It know these strings are temporary, I am not worried about memory, I am worried about efficiency of the heap copy of the string that temporarily is created during this call. If things were taking "const char *" before, copies were not being made. If they were being made, then they were probably being made for good reason. I would love to see the places where all of these copies were removed, but if they were "const char *" there were no copies going in, unless they needed to. If they didn't need to make the copies, then we can fix that function. _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
